-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Desktop state is read from window manager #80
Conversation
We should update commit message to "Desktop state is read from window manager" |
You should read about how good commit messages should look like. |
What's wrong with it? Would you come up with something more inspiring? We will see which is better? |
Reading your commit message, I have no idea what you did and why. |
Reading your comment, I have no idea what you want |
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:
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:
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. |
|
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. |
…ty. 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.
…ded Window Manager Hints 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.
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. |
Well, let's set up a commit guideline. A good commit message consists of
i.e.
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. |
Hello, have a look here |
@kurokawachan There is no need to open a new pull request. You can consolidate using your original branch:
Select the four commits as follows: pick Save, then edit the commit message by deleting everything except commit message #4, and save. Now Finally: |
Hello @ib Thanks for the suggestion. However, |
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. |
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. However, if a window is clicked directly, there is no tracking with wc->toggle_state, which means the variable will lose the information about the current state of the window manager. So, if available, get the current state by asking the window manager for it. Based on a patch by kurokawachan, github pull request #80, which has been rewritten to be more straightforward. This fixes github issue #79, reported by kurokawachan.
Aside from the fact that you don't want to customize the patch as discussed, it's also unnecessarily complicated. Fixed more straightforwardly. |
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" do you mean the the code I wrote? And it also is the SAME way all previous code wrote. If you look at file "plugins/wincmd.c" there are functions like
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 |
And your commit message is also confusing Here is your message
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. |
fixing issue #79