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

Do bounds-checking on more windows #4797

Merged
merged 17 commits into from
Dec 2, 2023
Merged

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Aug 28, 2023

Description

This is a continuation of #4740.

  • Bounds checking can be done on any window now.
  • BaseWindows can opt in to bounds-checking when they're shown (old behavior). This will prefer the monitor the cursor is on.
  • ChannelFilterEditorDialog now has the settings window as its parent (causing it to open there instead of the main window).

The following windows are now bounds-checked:

  • AccountSwitchPopup
  • UpdateDialog
  • SelectChannelDialog
  • SettingsDialog
  • QuickSwitcherPopup
  • ReplyThreadPopup
  • SearchPopup
  • QualityPopup
  • ColorPickerDialog
  • Viewer list
  • Tutorial popup
  • License popup

The following windows are not bounds-checked:

  • FramelessEmbedWindow
  • AttachedWindow (does its own positioning)
  • NotificationPopup (never constructed)
  • WelcomeDialog (never constructed)
  • InputCompletionPopup (it's supposed to be in the middle above the split input)

Fixes #4790.
Fixes #2660.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/settingspages/FiltersPage.cpp Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Don't refactor the BoundsChecker enum out of the class
Remove the moveWithinScreen function - leave that for another PR
Leave Off as a BoundsChecker option - it's not a lot of extra logic to handle and it's useful for testing bounds checking
Don't refactor the offset option in moveTo out - leave that for another PR
Remove the showEvent logic - it's not a sound cross-platform plan and if explored, it should be explored with a smaller subset of widgets that can be easily tested on multiple platforms in a separate PR

@8thony
Copy link
Contributor

8thony commented Aug 28, 2023

Viewer list can still get out of bound if the window is small

ColorPickerDialog can get out of bound if the settings window is close to the edge, and you click the change color option so the ColorPickerDialog has not enough space downwards, so it snaps out of bond at the top. (maybe you have to make the settings window smaller and higher, or use a smaller monitor)

InputCompletionPopup can get out of bounds if the emote name is long and the window too small, but it's fine, I guess

This PR Fixes #2660 as well

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Aug 28, 2023

Don't refactor the BoundsChecker enum out of the class
Remove the moveWithinScreen function - leave that for another PR

Such a PR would be ugly if it needed BaseWindow::BoundsChecker even though such a function wouldn't need to know about BaseWindow. I could have it in some namespace like widgets (window::moveTo(window, globalPoint, widgets::BoundsChecking::CursorScreen)).

Don't refactor the offset option in moveTo out - leave that for another PR

#4798

Remove the showEvent logic - it's not a sound cross-platform plan and if explored, it should be explored with a smaller subset of widgets that can be easily tested on multiple platforms in a separate PR

Where does this behave badly? It works fine on Windows and seemed to have worked on other platforms before #4740 as I can't find an issue related to this.


Viewer list can still get out of bound if the window is small

Right, it seems like the viewer list was supposed to be shown beneath the split header. This never happened.

ColorPickerDialog can get out of bound if the settings window is close to the edge, and you click the change color option so the ColorPickerDialog has not enough space downwards, so it snaps out of bond at the top. (maybe you have to make the settings window smaller and higher, or use a smaller monitor)

I can't really replicate that. The only thing that I observe is that about 8px are on another screen or OOB (I don't think I can fix that here, though).

@pajlada
Copy link
Member

pajlada commented Nov 27, 2023

Testing on Arch Linux & i3:

popup notes
AccountSwitchPopup ✅ works better than before. When opening it inbetween a monitor, it's now always 100% within screen bounds.
UpdateDialog Haven't tested yet
SelectChannelDialog ❌ Always opens in the top-left corner now
SettingsDialog ❌ Always opens in the top-left corner now
QuickSwitcherPopup ⚠️ Inconsistent, I had it open in the center as expected, and I had it open stuck to one side of the monitor.
ReplyThreadPopup ⚠️ Inconsistent, it now pops up in the middle of the screen before moving
SearchPopup ❌ Always opens in the top-left corner now
QualityPopup Haven't tested yet
ColorPickerDialog ❌ Always opens in the top-left corner now
Viewer list ⚠️ Change in behaviour, it used to just show up in the middle of the screen. Now it shows up in the middle of the screen, then it's moved to the split top-left corner
Tutorial popup ❌ Always opens in the top-left corner now
License popup ❌ Always opens in the top-left corner now

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Nov 27, 2023

Testing on Arch Linux & i3:

Can't reproduce any of this on Windows.

[Viewer list] then it's moved to the split top-left corner

That's where it's supposed to be.

ReplyThreadPopup | ⚠️ Inconsistent, it now pops up in the middle of the screen before moving

Shouldn't this behave the same as a user card? You can try and move the show() after the moveTo. At least on Windows, this breaks bounds-checking.

@pajlada
Copy link
Member

pajlada commented Nov 27, 2023

Yeah the issue on my system seems to come when we use show before moveTo

How come bounds checking isn't broken on Windows for something like the account switcher popup?

@pajlada
Copy link
Member

pajlada commented Nov 27, 2023

and the change in behaviour of the chatter list is fine, the ⚠️ was more for the "show up in middle then moved into position"

@pajlada
Copy link
Member

pajlada commented Dec 2, 2023

POST MY COMMIT
Testing on Arch Linux & i3:

popup notes
AccountSwitchPopup ✅ works better than before. When opening it inbetween a monitor, it's now always 100% within screen bounds.
UpdateDialog Haven't tested yet
SelectChannelDialog ✅ Opens in the center of the selected monitor as before
SettingsDialog ✅ Opens in the center of the selected monitor as before
QuickSwitcherPopup ✅ Opens in the center of the window as before
ReplyThreadPopup ⚠️ Inconsistent, it now pops up in the middle of the screen before moving
SearchPopup ✅ Opens in the center of the selected monitor as before
QualityPopup Haven't tested yet
ColorPickerDialog ✅ Opens in the center of the window as before
Viewer list ⚠️ Change in behaviour, it used to just show up in the middle of the screen. Now it shows up in the middle of the screen, then it's moved to the split top-left corner
Tutorial popup ✅ Opens in the center of the selected monitor as before
License popup ✅ Opens in the center of the selected monitor as before

This will call `show()` and `moveTo(...)` in different orders depending
on the platform
@pajlada
Copy link
Member

pajlada commented Dec 2, 2023

Testing on Arch Linux & i3:

popup notes
AccountSwitchPopup ✅ works better than before. When opening it inbetween a monitor, it's now always 100% within screen bounds.
UpdateDialog Haven't tested yet
SelectChannelDialog ✅ Opens in the center of the selected monitor as before
SettingsDialog ✅ Opens in the center of the selected monitor as before
QuickSwitcherPopup ✅ Opens in the center of the window as before
ReplyThreadPopup ✅ Opens on the cursor as before - does not do any bounds checking, also as before
SearchPopup ✅ Opens in the center of the selected monitor as before
QualityPopup Haven't tested yet
ColorPickerDialog ✅ Opens in the center of the window as before
Viewer list ⚠️ Change in behaviour, it used to just show up in the middle of the screen. Now it shows up in the middle of the screen, then it's moved to the split top-left corner
Tutorial popup ✅ Opens in the center of the selected monitor as before
License popup ✅ Opens in the center of the selected monitor as before

@pajlada
Copy link
Member

pajlada commented Dec 2, 2023

Testing on Arch Linux & i3:

popup notes
AccountSwitchPopup ✅ works better than before. When opening it inbetween a monitor, it's now always 100% within screen bounds.
UpdateDialog Haven't tested yet
SelectChannelDialog ✅ Opens in the center of the selected monitor as before
SettingsDialog ✅ Opens in the center of the selected monitor as before
QuickSwitcherPopup ✅ Opens in the center of the window as before
ReplyThreadPopup ✅ Opens on the cursor as before - does not do any bounds checking, also as before
SearchPopup ✅ Opens in the center of the selected monitor as before
QualityPopup Haven't tested yet
ColorPickerDialog ✅ Opens in the center of the window as before
Viewer list ✅ Change in behaviour, it used to just show up in the middle of the screen. Now it's moved to the split top-left corner
Tutorial popup ✅ Opens in the center of the selected monitor as before
License popup ✅ Opens in the center of the selected monitor as before

@pajlada
Copy link
Member

pajlada commented Dec 2, 2023

update dalog thing on macos seems to work as expected

image

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Dec 2, 2023

Windows 10:

Popup Status
AccountSwitchPopup
UpdateDialog
SelectChannelDialog
SettingsDialog
QuickSwitcherPopup
ReplyThreadPopup
SearchPopup
QualityPopup
ColorPickerDialog
Chatter list
Tutorial popup
License popup

@pajlada pajlada merged commit c4c9447 into Chatterino:master Dec 2, 2023
18 of 19 checks passed
@Nerixyz Nerixyz deleted the fix/movetos branch December 2, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants