Try let the window manager move the window, fixes #429 #445

nuttyartist · 2023-01-15T18:34:25Z

This should fix #429. Can you please review if the code is satisfactorily written?

nuttyartist · 2023-01-15T18:35:27Z

Whoops, I see I'm uploading here CMakeLists.txt.user. Is it not in gitignore?

guihkx · 2023-01-15T18:47:23Z

Whoops, I see I'm uploading here CMakeLists.txt.user. Is it not in gitignore?

It seems that file is generated by QtCreator (which I personally don't use), but it should be fine to add it to .gitignore.

CMakeLists.txt.user

src/mainwindow.cpp

bjorn

Note that you'll want to do the same change to the UpdaterWindow.

bjorn · 2023-01-18T20:29:21Z

src/mainwindow.cpp

-        int dy = event->globalY() - m_mousePressY;
-        move(dx, dy);
-
+        if (!window()->windowHandle()->startSystemMove()) {


I have no idea whether it is fine to continuously call this function on all platforms. Or does mouseMoveEvent stop being called when this function succeeded? The documentation doesn't say anything about it, so I guess we could just test.

Alternatively, this function could be called in MainWindow::mousePressEvent instead, where we maybe could do m_canMoveWindow = !window()->windowHandle()->startSystemMove() instead of just assigning true (untested)

nuttyartist · 2023-01-19T11:53:09Z

My C++ and Qt skills are quite rusty. If someone wants to take that PR, I'd be glad 😅

Also, why is after adding CMakeLists.txt.user to gitignore the file is still added?

Note that you'll want to do the same change to the UpdaterWindow.

Good catch. I tried but with no success. I got member access into incomplete type QWindow

bjorn · 2023-01-19T12:16:04Z

My C++ and Qt skills are quite rusty. If someone wants to take that PR, I'd be glad 😅

You're maintaining this thing, I think I'll let you the opportunity to get a little bit less rusty. ;-)

Also, why is after adding CMakeLists.txt.user to gitignore the file is still added?

Because you added it to .gitignore after it was already added to git. Changing .gitignore is not going to rewrite history for you. :-) To remove that file from git without removing the file from your workspace, you can use git rm --cached CMakeLists.txt.user and commit that.

member access into incomplete type QWindow

It means you need to add #include <QWindow> to the affected .cpp file.

nuttyartist · 2023-01-20T15:38:07Z

You're maintaining this thing, I think I'll let you the opportunity to get a little bit less rusty. ;-)

Haha fair. Thanks for the tip. Seems to work now. Let me know if it's fine.

@zjeffer Thanks 😉

Fixes #429

zjeffer · 2023-01-21T18:55:50Z

I fixed the qt5.9 build, rebased and squashed the commits.

@mominul can you test this on wayland by downloading the latest build from here: https://github.com/nuttyartist/notes/actions/runs/3976090004. At the bottom, you can download the AppImages for both Qt5 and Qt6.

guihkx · 2023-01-21T20:14:50Z

The AppImages currently don't include the Wayland platform plugin, so it will just fallback to XWayland.

However, I think it's just a matter of installing the qtwayland5 / qtwayland6 packages and the linuxdeploy tool will pick up those plugins automatically.

If that doesn't work, we'll have to find a way to include the Wayland plugin manually.

I would add those changes myself, but I'm not home at the moment...

@zjeffer Feel free to experiment, if you want. If that doesn't work, I can pick it up from where you left later.

nuttyartist · 2023-01-22T19:17:38Z

Feel free to merge this when it's ready.

guihkx · 2023-01-22T19:51:08Z

Okay, I think we aren't quite there yet regarding Wayland support in AppImages. And even people much smarter than me seem to agree:

linuxdeploy/linuxdeploy-plugin-qt#25

First, because it's a struggle to bundle all the required Wayland libraries. Even though I succeeded (to the best of my knowledge, anyway).

But I either experienced random, weird behavior, like this one:

wayland.mp4

... Or just straight up the crashing of the whole desktop (wtf, GNOME?):

Jan 22 16:23:28 arch-des gnome-shell[798]: Buggy client caused popup to be placed outside of parent window
Jan 22 16:23:29 arch-des gnome-shell[798]: **
Jan 22 16:23:29 arch-des gnome-shell[798]: libmutter:ERROR:../mutter/src/wayland/meta-wayland-popup.c:233:meta_wayland_popup_grab_get_top_popup: assertion failed: (!wl_list_empty (&grab->al>
Jan 22 16:23:29 arch-des gnome-shell[798]: Bail out! libmutter:ERROR:../mutter/src/wayland/meta-wayland-popup.c:233:meta_wayland_popup_grab_get_top_popup: assertion failed: (!wl_list_empty >
Jan 22 16:26:46 arch-des systemd-coredump[2336]: [🡕] Process 798 (gnome-shell) of user 1001 dumped core.

Even if those are rare and I was just unlucky, I don't think it's something we want to ship to users, considering that the Xbc backend still works and offers a better experience overall.

guihkx · 2023-01-22T19:52:32Z

Regarding the changes itself, I'm experiencing some very weird behavior while testing the Qt 5 build on Linux (not running on Wayland)...

After moving the window for a bit (and then releasing it), the mouse cursor will flicker like crazy sometimes:

weird-behavior-after-moving-window.mp4

Not only that, clicking on the text editor makes the window manager think we are grabbing the window again, lol.

Further testing revealed bugs

zjeffer · 2023-01-22T20:18:53Z

I realized I'm also getting some strange bugs:

notes-2023-01-22_21.06.51.mp4

Here's me with this PR's branch on bspwm, which uses X11. Resizing with bspwm's keyboard shortcuts works fine: I press super+left click on the window to move, super+right click near a corner to resize.

Moving the window using the window bar works fine. I don't get the bug you're having.
Resizing the window by dragging the top border continuously moves the window diagonally to the left until I release the mouse button. Only the top border seems to be interactive.
- Resizing windows by dragging the border shouldn't even be possible in most tiling window managers. I've been using bspwm for around 5 years, and I've never come across an app that can be resized by dragging one of the window's borders. I always resize using keyboard shortcuts.
With native window frame disabled, double clicking the bar will create new borders around the whole window (see 0:40 in the video). This makes all borders have resizing handles, including the corners for diagonal resizing. Dragging these handles results in the same bug as point 2.
Resizing this way widens the notes list pane to be about half the window width.

EDIT: Oops, looks like this behaviour has been present for a while. The aur's latest version and the dev version both have these bugs on bspwm. I'll create a separate issue. I never use this app in floating mode so I've never noticed this behaviour before.

guihkx · 2023-01-22T20:24:23Z

LOL, that's hilariously broken. We'll need to investigate some more...

nuttyartist · 2023-01-25T09:58:00Z

So is it not preferable to let the window manager handle the move operation if supported?

guihkx · 2023-01-25T21:17:28Z

So is it not preferable to let the window manager handle the move operation if supported?

It definitely is, IMO. It's just that it's probably broken thanks to our own window management code, so we should fix it first.

nuttyartist · 2023-02-06T08:53:59Z

So what's left here to fix? If this solution is still better than our previous one, I think we can merge this and open a new issue.

nuttyartist · 2023-02-06T09:27:54Z

This is the only issue that currently remains for completing version 2.1.0.

This provides more direct feedback (mouse changes to move cursor on press already on GNOME), avoids repeated calls in case the system move is not supported and simplifies the code a little.

bjorn · 2023-02-06T11:55:24Z

So what's left here to fix? If this solution is still better than our previous one, I think we can merge this and open a new issue.

I've pushed a change that addresses my comment.

This is the only issue that currently remains for completing version 2.1.0.

While ideally we'd also improve / fix the resize behavior in 2.1, I think this PR is ready to be merged.

guihkx · 2023-02-07T02:56:16Z

Right now this change only affects non-Windows builds, but I think we should also add this functionality on Windows, which appears to be done here instead:

notes/src/mainwindow.cpp

Lines 2806 to 2814 in 1471e87

    
           void MainWindow::mouseMoveEvent(QMouseEvent *event) 
        
           { 
        
               if (m_canMoveWindow) { 
        
                   //        this->setCursor(Qt::ClosedHandCursor); 
        
                   int dx = event->globalPos().x() - m_mousePressX; 
        
                   int dy = event->globalY() - m_mousePressY; 
        
                   move(dx, dy); 
        
               } 
        
           }

guihkx · 2023-02-07T04:54:03Z

Ah, the Windows code was added elsewhere, my bad.

LGTM!

bjorn · 2023-02-07T10:27:27Z

Right now this change only affects non-Windows builds, but I think we should also add this functionality on Windows, which appears to be done here instead:

Yeah, in general I think we could do better in unifying certain code paths, but for now i just adjusted both versions of the mousePressEvent.

nuttyartist · 2023-02-08T14:46:11Z

Tested and works. Merging... Thanks everybody!

nuttyartist requested review from bjorn, guihkx and zjeffer January 15, 2023 18:34

zjeffer requested changes Jan 15, 2023

View reviewed changes

CMakeLists.txt.user Outdated Show resolved Hide resolved

src/mainwindow.cpp Outdated Show resolved Hide resolved

guihkx reviewed Jan 15, 2023

View reviewed changes

src/mainwindow.cpp Outdated Show resolved Hide resolved

bjorn reviewed Jan 18, 2023

View reviewed changes

Fix moving the window on Wayland

0caa228

Fixes #429

zjeffer force-pushed the native-window-move branch from 343212a to 0caa228 Compare January 21, 2023 18:35

zjeffer previously approved these changes Jan 21, 2023

View reviewed changes

bjorn mentioned this pull request Jan 26, 2023

Resizing on tiling window managers is broken #454

Open

Start system move on mouse press instead of move

8b4e038

This provides more direct feedback (mouse changes to move cursor on press already on GNOME), avoids repeated calls in case the system move is not supported and simplifies the code a little.

bjorn approved these changes Feb 6, 2023

View reviewed changes

guihkx approved these changes Feb 7, 2023

View reviewed changes

nuttyartist merged commit 3253eb9 into dev Feb 8, 2023

bjorn deleted the native-window-move branch February 8, 2023 16:05

guihkx mentioned this pull request Feb 13, 2023

Qt 5 "deactivates" the window once you stop moving it around #472

Open

guihkx mentioned this pull request Aug 10, 2023

Add wayland support to appimages #633

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try let the window manager move the window, fixes #429 #445

Try let the window manager move the window, fixes #429 #445

nuttyartist commented Jan 15, 2023

nuttyartist commented Jan 15, 2023

guihkx commented Jan 15, 2023

bjorn left a comment

bjorn Jan 18, 2023

nuttyartist commented Jan 19, 2023

bjorn commented Jan 19, 2023 •

edited

Loading

nuttyartist commented Jan 20, 2023

zjeffer commented Jan 21, 2023

guihkx commented Jan 21, 2023

nuttyartist commented Jan 22, 2023

guihkx commented Jan 22, 2023

guihkx commented Jan 22, 2023

zjeffer commented Jan 22, 2023 •

edited

Loading

guihkx commented Jan 22, 2023

nuttyartist commented Jan 25, 2023

guihkx commented Jan 25, 2023

nuttyartist commented Feb 6, 2023

nuttyartist commented Feb 6, 2023

bjorn commented Feb 6, 2023

guihkx commented Feb 7, 2023

guihkx commented Feb 7, 2023

bjorn commented Feb 7, 2023

nuttyartist commented Feb 8, 2023

Try let the window manager move the window, fixes #429 #445

Try let the window manager move the window, fixes #429 #445

Conversation

nuttyartist commented Jan 15, 2023

nuttyartist commented Jan 15, 2023

guihkx commented Jan 15, 2023

bjorn left a comment

Choose a reason for hiding this comment

bjorn Jan 18, 2023

Choose a reason for hiding this comment

nuttyartist commented Jan 19, 2023

bjorn commented Jan 19, 2023 • edited Loading

nuttyartist commented Jan 20, 2023

zjeffer commented Jan 21, 2023

guihkx commented Jan 21, 2023

nuttyartist commented Jan 22, 2023

guihkx commented Jan 22, 2023

guihkx commented Jan 22, 2023

zjeffer commented Jan 22, 2023 • edited Loading

guihkx commented Jan 22, 2023

nuttyartist commented Jan 25, 2023

guihkx commented Jan 25, 2023

nuttyartist commented Feb 6, 2023

nuttyartist commented Feb 6, 2023

bjorn commented Feb 6, 2023

guihkx commented Feb 7, 2023

guihkx commented Feb 7, 2023

bjorn commented Feb 7, 2023

nuttyartist commented Feb 8, 2023

bjorn commented Jan 19, 2023 •

edited

Loading

zjeffer commented Jan 22, 2023 •

edited

Loading