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

Fix toolbar toggling when entering/leaving fullscreen mode #3410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shamazmazum
Copy link
Contributor

This PR fixes toolbar toggling when entering/leaving fullscreen, as the title says. This is what it looks like before the patch:

Leaving fullscreen mode:
20240605_10h15m17s_grim

Entering fullscreen mode:
20240605_10h15m00s_grim

When you press f11 (toggle-fullscreen) your toolbar does not disappear in the fullscreen mode and disappears when you leave it. When you toggle fullscreen by external means (e.g. pressing Mod+f in sway), nothing happens with the toolbar.

This patch assures that toolbar disappears in the fullscreen mode and appears in "the normal mode" in following scenarios:

  • When toggle-fullscreen is invoked
  • When toggling fullscreen via X11 WM /Wayland compositor
  • When pressing fullscreen button on Youtube.

@aadcg
Copy link
Member

aadcg commented Jun 6, 2024

I'm not sure I agree with the suggested behaviour.

I see that the message and status bars disappear when exiting fullscreen, which seems like a bug. Invoking toggle-fullscreen or toggling the fullscreen state via X11/Wayland must be equivalent operations.

@shamazmazum
Copy link
Contributor Author

@aadcg I think, I don't understand something. This patch makes toolbar behavior consistent, no matter how you enter/leave fullscreen mode. Currently it's inconsistent: entering/leaving fullscreen via X11/Wayland does not affect toolbar at all. Also this patch fixes the bug you mention.

Invoking toggle-fullscreen or toggling the fullscreen state via X11/Wayland must be equivalent operations.

This patch does exactly that.

@aadcg
Copy link
Member

aadcg commented Jun 7, 2024

@shamazmazum, I had a very brief look at your PR yesterday since I'm currently unavailable. I'll get back to it after the 12th of June.

My point was that entering fullscreen should not hide the message and status buffers. Anyway, I'll study the current behavior and your changes in depth as to ensure that I don't miss any detail. Thanks for your interest in the project!

@shamazmazum
Copy link
Contributor Author

My point was that entering fullscreen should not hide the message and status buffers.

You have this is windows.lisp:

(define-command toggle-fullscreen (&key (window (current-window))
                                   skip-renderer-resize)
  "Fullscreen WINDOW, or the current window, when omitted.
When `skip-renderer-resize' is non-nil, don't ask the renderer to fullscreen the window."
  (let ((fullscreen (fullscreen-p window)))
    (unless skip-renderer-resize
      (if fullscreen
          (ffi-window-unfullscreen window)
          (ffi-window-fullscreen window)))
    (toggle-status-buffer :show-p (not fullscreen))
    (toggle-message-buffer :show-p (not fullscreen))))

There is a code for toggling, it just works incorrectly.

But OK, i won't bother you now :)

@aadcg
Copy link
Member

aadcg commented Jun 7, 2024

@shamazmazum I see your point now. You're saying that commit 36d15f9 got the logic wrong, right?

My point is that commit 746f2fe is a rather odd UI decision (сомнительно точно, и наверно не ОК). In my view, hiding the status and message buffers are orthogonal to fullscreen events. Note that we have the command toggle-toolbars, which hides them. Since these 2 buffers are part of the Nyxt UI, the user alone should be the one to make the choice. If we draw a parallel with Emacs, the same conclusion would follow. Does it seem reasonable that fullscreening Emacs would hide the modeline? I don't think so.

If @shamazmazum and @jmercouris agree, I'd suggest changing the default behavior.

@shamazmazum
Copy link
Contributor Author

You're saying that commit 36d15f9 got the logic wrong, right?

Yes.

In general, I can agree with you and toggling fullscreen may just as well do what it has to do with fullscreen and leave the toolbar be. I can only suggest that a rationale for hiding the toolbar is to enable some kind of reading mode where you have only your content and nothing else. Personally I never use anything like that, I just accidentally pressed f11 and noticed a strange behaviour. It definitelly needs to be fixed, but how is up to you.

@aadcg
Copy link
Member

aadcg commented Jun 14, 2024

I can only suggest that a rationale for hiding the toolbar is to enable some kind of reading mode where you have only your content and nothing else.

I agree, but let's leave this idea for the future.

Personally I never use anything like that, I just accidentally pressed f11 and noticed a strange behaviour. It definitelly needs to be fixed, but how is up to you.

Indeed. Let's go with my suggestion, i.e. agree that going fullscreen has nothing to do with hiding the status and message buffers. We can proceed as if @jmercouris has agreed with the change.

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

Successfully merging this pull request may close these issues.

None yet

2 participants