Skip to content
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

Make display of window buttons in top bar user configurable #70

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marcohu
Copy link
Contributor

@marcohu marcohu commented May 3, 2016

I had some problems with git and needed to reset master. It was easier then to create a new PR. Sorry for that.

Here is another one that provides the same, minus localization, but plus the extended set of button position options. Tested on 3.18.

Tab width has been set to '2'. I assume this is your preference?

fixes #35
fixes #46

showButtons();
}
}));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per your request, the wiring now happens in the buttons module.

If I understood you correctly, the options should also be loaded here. This implies that we have to perform cleanup as well. Generally not a problem, but as more modules use the options, they all have to perform cleanup if they listen to settings changes.

I would therefore suggest that at least the option loading should move to the Extension module as then cleanup has to performed only once (and you don't have to remember the constraint to cleanup options if you listen for changes).

@deadalnix
Copy link
Collaborator

For some reason i missed the notification that the PR was updated. So thanks @gsantner .

i'll be reviewing this over the week end.

@bilelmoussaoui
Copy link

Is there a chance to get this merged? 👍

@deadalnix
Copy link
Collaborator

Mmmmh so there was another very similar PR I got confused.

@gsantner I want this actually. I'm just swamped.

@brianc118
Copy link

Would love to see this get merged

@deadalnix
Copy link
Collaborator

This is very similar to #53 and same comment apply. This is a kitchen sink: add 3rd party code, add translation support, add a build system, add configuration screen, and add the feature for the buttons.

The reason this is hanging for so long is that i only have limited time to review this, and this is made is such a way that I simply can't review it with limited time, because of the above.

So I propose this is split is such a way:
1/ Add the feature with a hardcoded variable to control it.
2/ Add the config screen, wire it is to feed the right value.
3/ Add translation support.

This is better done as separate PRs.

I'll focus on 1/ so far. There are 4 options, but 2 of them apply on left side buttons. There is already some support for button on the left side, if gnome's configuration says so. Therefore, I don't think these options makes sense for right hand side buttons.

@alci63
Copy link

alci63 commented Jun 8, 2017

Option to remove the buttons completely makes real sense: closing can be done with the drop down menu on the title, (un)maximizing by draging the window, and of course there are keyboard shorcuts for all that. So there is really no need for these buttons, is there ?

@deadalnix
Copy link
Collaborator

But then, you don't need these buttons either on every window and can remove them from your gnome config.

@alci63
Copy link

alci63 commented Jun 8, 2017

@deadalnix Well, I didn't think of that ! :-) But, then when not maximized, I have no close option (expect kb shortcuts), since it's added in the drop down menu in the application's title when maximized only.

@franglais125
Copy link

Sorry for the spam, but this might be useful to some. A fork including this PR is available on e.g.o: https://extensions.gnome.org/extension/1267/no-title-bar/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants