lxde
Desktop state is read from window manager
We should update commit message to
"Desktop state is read from window manager"
What's wrong with it?
Would you come up with something more inspiring? We will see which is better?
Hi @kurokawachan thanks for the change. To make the purpose of the change clear to other developers, I'd suggest rewriting the commit message to something like this:
Check whether desktop is shown using `NET_SHOWING_DESKTOP` X11 property.
To correctly reflect the desktop showing state in the UI, the current state needs to be
read from X11 properties set by the window manager. Otherwise the UI may not always
be updated when desktop showing state is changed by other applications.
The above is just an example. It may not correctly describe what you did in the change.
There's no style guide for LXDE at the moment, but a commonly seen format is:
- Describe what you did in the change on the first line.
- Then, explain why this is needed.
This can help other collaborators understand the purpose of the change and speed up code review. Thank you!
Hi @ib I totally agree with you that the commit message needs to better describe why the change is needed. As not everyone is good at doing this, would you work with @kurokawachan to polish the commit message, and then get the fix merged, please? The lack of bug fixes may prevent lxpanel from being included in major distros (Debian for example). Really appreciate that. Thank you both!
@PCMan
- I already did work with kurokawachan on commit messages in #78. He doesn't seem to be interested. Nevertheless, I am of course considering his suggestions.
- The plan is to fix all GTK3 issues and as many bugs as possible before the trixie freeze deadline, so that LXDE will still be available in Debian.
- BTW, did you read my email of 2024-12-14 regarding my lack of permission to set up reposity features? It would really help to have issues enabled for all LXDE repositories.
Hello @PCMan
Thanks for the clarification. I do believe the message you wrote is better understandable. Let me see how to get the fix merged.
Hello @ib
If you agree with the updated message, would you merge it?
Thanks
Hi @ib Thank you I just enabled Issues for all repos. @kurokawachan Thank you for the update. The commit looks good to me, but let's wait for @ib 's review since he's the current maintainer. Really appreciate what both of you did to keep this project running!
There's no style guide for LXDE at the moment
Well, let's set up a commit guideline.
A good commit message consists of
- a header line that describes what is being done
- a body that explains why it is being done
i.e.
Describe the commit in one line
The body of the commit message is a few lines of text, explaining things
in more detail, possibly giving some background on the problem being
fixed.
It can be several paragraphs, with proper line breaks.
If there is a reference, end it with something like:
This fixes url-of-whatever-it-fixes.
A line must not exceed 72 characters, and the header is just one line with the verb in the imperative and no sentence-ending period.
Over time, commits should tell a story about the history of a repository and how it came to be the way that it currently is.
@kurokawachan Additional note: Consolidate changes belonging to one issue into a single commit, performing squash and/or amend operations in your working repository if necessary.
2d84b94 and fd75d28, for example, do not belong in the history.
@kurokawachan Your issue #79 explains why and what excellently. This is what the commit message should say.
Consolidate changes belonging to one issue into a single commit
Hello, have a look here https://github.com/lxde/lxpanel/pull/95
@kurokawachan There is no need to open a new pull request. You can consolidate using your original branch:
git checkout desktop_state
git rebase -i HEAD~4
Select the four commits as follows:
pick squash squash squash
Save, then edit the commit message by deleting everything except commit message #4, and save.
Now git commit --amend to edit your commit message according to https://github.com/lxde/lxpanel/pull/80#issuecomment-2656884091.
Finally: git push --force which will update this pull request.
Hello @ib Thanks for the suggestion. However, git push --force is not good practice
Would you just merge the new one?
@kurokawachan
However,
git push --forceis not good practice
It's good practice for a temporary working branch for a pull request that will be deleted afterwards. We are not talking about the published master or main branch.
Aside from the fact that you don't want to customize the patch as discussed, it's also unnecessarily complicated.
Fixed more straightforwardly.
Aside from the fact that you don't want to customize the patch as discussed
Why are you gaslighting me into believing I am not very cooperative? I already did modify the commit message. What are you referring to?
it's also unnecessarily complicated
"it" do you mean the the code I wrote? get_net_showing_desktop this function that I wrote free the memory so the user would not need to worry about memory leak. It encapsulate the memory management from the user. How is it "unnecessarily complicated"? I saw you removed the function in the your "fixed" version. I don't think that is good code.
And it also is the SAME way all previous code wrote. If you look at file "plugins/wincmd.c" there are functions like
get_net_wm_desktop
get_net_wm_pid
get_net_wm_state
They all have same style. I designed the code to match the style of previous code. How it is unnecessarily complicated?
Here is side-by-side
The code I wrote https://github.com/lxde/lxpanel/pull/95/commits/be42587e1ecf812cdb33c59712e02d0b6e569182 And your "fixed" https://github.com/lxde/lxpanel/commit/93890c1ecfe54edc3b2b2a23f163415d7f0a2c12
And your commit message is also confusing
Here is your message
The desktop state (showing the desktop, i.e. all windows are hidden, or
windows are visible) is tracked with wc->toggle_state, a value that is
toggled each time the windows are minimized or restored.
The wording is just off. A value that is NOT toggled each time the windows are minimized or restored. And this is the whole point to fix the bug.
It is, however, A value that is toggled each time the button being clicked, however not necessarily the windows are minimized or restored.
I saw you rewrite all the commit message I wrote. To improve things? Does not look like it.