Closed freimair closed 8 months ago
diegodario88
commented
9 months ago By the way, it's great to see contributions here! 😊
@freimair, thank you for your proposed changes, and please don't hesitate to ask if you have any questions or need assistance.
freimair
commented
8 months ago @diegodario88 You made the always-on-top configurable which lead to a merge conflict.
Are you sure the always-on-top is needed? please see the commit description on my reasoning why it might not be.
Just making sure not to bloat the code for something that is not necessary...
diegodario88
commented
8 months ago Hey @freimair ,
I appreciate your feedback and the points you've raised. Let's keep the conversation going to make sure we're on the same page.
@diegodario88 You made the always-on-top configurable which lead to a merge conflict.
You mentioned the always-on-top feature and the potential merge conflict. To clarify, I believe there might be some confusion with another merge request that I suggested for rebasing.
Are you sure the always-on-top is needed? please see the commit description on my reasoning why it might not be.
Regarding the necessity of the always-on-top feature, I understand your concerns. However, I believe that allowing users to choose is a positive approach. Customization can be a valuable asset, even if our goal is to maintain a clean codebase.
Just making sure not to bloat the code for something that is not necessary...
I'd like to address your concern about bloating the module. In my opinion, adding the always-on-top feature won't overly complicate it. Additionally, one could make a similar argument about the animation time configuration, as it already aligns with the system's default settings for animation time.
freimair
commented
8 months ago Hey @freimair ,
I appreciate your feedback and the points you've raised. Let's keep the conversation going to make sure we're on the same page.
@diegodario88 You made the always-on-top configurable which lead to a merge conflict.
sorry, started off on the wrong foot there maybe. No offence! Never! Just a guy thinking about stuff :).
You mentioned the always-on-top feature and the potential merge conflict. To clarify, I believe there might be some confusion with another merge request that I suggested for rebasing.
Actually, this PR could have been merged without conflict on creation. You then added the always-on-top configuration and that caused this PR to not merge without conflict. That is how I stumbled over your change in the first place. However, no worries here.
Are you sure the always-on-top is needed? please see the commit description on my reasoning why it might not be.
Regarding the necessity of the always-on-top feature, I understand your concerns. However, I believe that allowing users to choose is a positive approach. Customization can be a valuable asset, even if our goal is to maintain a clean codebase.
Letting people choose is exactly how I feel, no worries here, either.
The question I raise is simply about whether the feature can actually be used. Here a list of use cases I see happening:
1) when Auto-Hide is active, the terminal hides as soon as I do something outside of the terminal window, be it opening a new app or focusing another window. So always-on-top has no effect with auto-hide on or does it? At least on my system, I see the this behavior. 2) with Auto-Hide off/always-on-top on and I launch a new app or change to another already open app window, the newly selected window stays below the terminal and I have to press the hotkey to hide the terminal in order to use the window I just selected (where I already had to interact with the system - [Alt][Tab], [Super][search text][Enter], ...) 3) ? is there a use case where always-on-top has benefits? (I am asking :), because I do not see a case so far)
Just making sure not to bloat the code for something that is not necessary...
I'd like to address your concern about bloating the module. In my opinion, adding the always-on-top feature won't overly complicate it. Additionally, one could make a similar argument about the animation time configuration, as it already aligns with the system's default settings for animation time.
No, of course it does not overly complicate it. But when it comes to maintenance, every line of code one does not have to consider is a good line of code. And as we all know, code smell accumulates quicker than we like...
Alas, I was just not seeing a use case (3?), where always-on-top makes actual sense. Maybe you or anyone else can fill me in.
freimair
commented
8 months ago https://github.com/diegodario88/quake-terminal/pull/10/commits/16650327f17e15f5cac1f1e6c1946a69f644fce0 has been fixed by https://github.com/diegodario88/quake-terminal/commit/0c745dda7a690bcabe701b90784862b8555009eb already.
https://github.com/diegodario88/quake-terminal/pull/10/commits/b8250483626efbefe37378e8fdd3ab7f40e9309d has been superseded by https://github.com/diegodario88/quake-terminal/commit/0c745dda7a690bcabe701b90784862b8555009eb as well.
diegodario88
commented
8 months ago is there a use case where always-on-top has benefits? (I am asking :), because I do not see a case so far)
Hey @freimair I can definitely see a use for the always-on-top feature.
There have been instances when I've found it quite handy. For example, when I'm working with tmux and have auto-hide enabled, the terminal window tends to disappear after copying something. Having the 'always-on-top' option helps keep it in view
Streamlines the hiding/stay on top/focus behavior of the extension.
Tested on Gnome Shell 45.0, Arch Linux, X11 and Wayland, 4-Monitor setup.