-
Notifications
You must be signed in to change notification settings - Fork 151
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
Implement Decorate/Undecorate #1725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, but it seems straightforward, respects my preference, and maintains backward compatibility.
I disagree that your other approach is cleaner, because it adds a lot of duplicate options to achieve the same effect. If you really think that there is merit in providing a way to implement your intended decorative scheme for SSD only, I'd suggest that the cleanest approach is this implementation, with an additional flag for Decorate
, Undecorate
and ToggleDecorations
that would optionally limit the action to SSD only.
First of all, thanks for your 2 PRs. What I am not sure about is the
This indeed feels weird. Does Undecorate actually need the argument if we have one for I'll look into both PRs in more details in the coming days. |
Note that it adds more options, but needs less code to do that. For me it is easier to grasp and has fewer unexpected edge cases. But it also requires a deeper understanding of the low-level details (e.g. the difference between SSD and CSD). I also don't think the previous approach was perfect. I have hope that we can find an even better approach that is both clear and high level and also doesn't break backwards compatibility. Then again, I believe that breaking backwards compatibility might be justified in some cases. The 3-state What is your general position on breaking changes? Should I try to come up with a proposal or can I save myself the trouble? |
We usually try pretty hard to prevent breaking changes for user facing configuration, same for UX patterns. To save some work it is always suggested to discuss some approach beforehand. |
Remember that there is more to "clean" than lines of code. If we have a lot of overlap with the effects of actions, it muddies the user interface for configuring these things, and also allows for weird and buggy interactions when chaining together multiple actions that partially overlap. This produces more edge cases, not fewer. Simplifying the action list and adding a few options to make user intent more clear is worth a few extra lines to parse and interpret the actions. I'm not really advocating for exposing SSD/CSD limiters in the config; I only suggested that this is a possibility if the distinction is appropriate. After thinking some more, I think the best option is to leave a |
Just for completeness, there were ideas floating around at some point to add something like |
Closed in favor of #1733 |
This is an alternative approach for #1724 as proposed by @ahesford. This approach is so different that I thought a new pull request makes more sense.
This keeps backwards compatibility and thereby also the confusion about SSD and titlebar. I still think the other approach is cleaner. But as far as I was able to figure out, both can accomplish the same.
I was not sure how to handle the situation in which a window has no SSD and a user uses
Undecorate
withkeepBorder="yes"
: My first instinct was to make this idempotent, i.e. you end up in the same state no matter where you started from, in this case: borders. On the other hand, it is a bit weird that a call toUndecorate
actually adds decorations.My original motivation for this is that I want to remove title bars by default (without adding borders to CSD windows). This is only possible with the latter approach. It is still possible to get something idempotent by calling
Decorate
first. So that's what I ended up with.I hope you get why I think the other approach is cleaner.