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

Add option to hide the window title on macOS #1837

Merged
merged 1 commit into from Aug 2, 2019

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Jul 24, 2019

See #1072.
Don't merge this yet please.
I have a couple questions:
Do I need the autoreleasepool in cocoa_hide_window_title()?
Why are there seemingly two places for the deprecated config options? One below

@deprecated_handler('macos_show_window_title_in_menubar')

and one here:

if not getattr(opts, 'macos_show_window_title_in_menubar', True):
    opts.macos_show_window_title_in = 0b01

Is there some way to have one config option but two independent variables where that option is stored? macos_show_window_title_in_menubar and macos_show_window_title_in_window are really two different, independent options. I agree that having one config option for the two things is better but in the code I'd like to treat them separately, so I don't need to use bit masks and bit shifts when I'm using the options in the code. The way I partially solved it now is a bit ugly, still requires not-so-readable bit operations and doesn't allow me to access macos_show_window_title_in_menubar and macos_show_window_title_in_window from the python code. It also doesn't allow me to properly handle the case where both macos_show_window_title_in and macos_show_window_title_in_menubar are present in the config.

@Luflosi
Copy link
Contributor Author

Luflosi commented Jul 24, 2019

The CI complains about the option macos_show_window_title_in_menubar being referenced in the changelog. How do I fix that?

@kovidgoyal
Copy link
Owner

Remove the :opt: prefix before it in the changelog

@kovidgoyal
Copy link
Owner

yes, it is best to have the autoreleasepool because this function is called during window construction which happens before the event loop is started.

No you dont need to do it in two places.

As for option parsing use option_type=choices('all', 'window', 'menubar') and in state.c create a parser function that sets an actual bit field.

@Luflosi
Copy link
Contributor Author

Luflosi commented Jul 31, 2019

As for option parsing use option_type=choices('all', 'window', 'menubar') and in state.c create a parser function that sets an actual bit field.

Implementing it like that will still require me to have only one variable for both options and access them by accessing the correct bit in bit field. I'd prefer to avoid that. Is there another way that would allow me to use two variables directly in the config parser code?

@kovidgoyal
Copy link
Owner

No, there isn't.

@Luflosi Luflosi marked this pull request as ready for review August 2, 2019 03:35
@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 2, 2019

Sorry that I marked this as ready for review, this isn't actually working properly yet.

Deprecate `macos_show_window_title_in_menubar` and create a new option `macos_show_window_title_in`.
@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 2, 2019

Please review.

@kovidgoyal kovidgoyal merged commit b5c2163 into kovidgoyal:master Aug 2, 2019
@Luflosi Luflosi deleted the macos_show_window_title_in branch August 2, 2019 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants