atlas-engineer / nyxt

Nyxt - the hacker's browser.
https://nyxt-browser.com/
9.64k stars 404 forks source link

Fix toolbar toggling when entering/leaving fullscreen mode #3410

Open shamazmazum opened 3 weeks ago

shamazmazum commented 3 weeks ago

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:

aadcg commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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

My point is that commit 746f2fe78c42b2023a535ef1722e5a43ffce9184 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 commented 2 weeks ago

You're saying that commit https://github.com/atlas-engineer/nyxt/commit/36d15f984ee7130687ac2423f00d0a46c2e33933 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 commented 1 week ago

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.