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

Toggle Mouse Resizing #274

Merged
merged 4 commits into from
Jul 27, 2024
Merged

Toggle Mouse Resizing #274

merged 4 commits into from
Jul 27, 2024

Conversation

Darth-Raazi
Copy link
Contributor

@Darth-Raazi Darth-Raazi commented Jul 16, 2024

Description

Added toggle to enable/disable mouse resizing, cleaned up dashboard locking, cleaned up menu bar

This PR closes #XXX.

Developer Testing

Here's what I did to test my changes:

  • Ran A
  • Ran B
  • Ran C

Reviewer Testing

Here's what you should do to quickly validate my changes:

  • Run D and check output

This change is Reviewable

Copy link
Member

@patrick-gu patrick-gu left a comment

Choose a reason for hiding this comment

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

Some comments about impl.

Also, I think mouse resizing remains very weird:

  • If you click on something, it'll follow you around, right? It keeps doing this if you turn off mouse resizing
  • Or if you click on something then remove all, your mouse still has the grab hand
  • The resize corners are still there

Finally I think remove all items is pretty dangerous to have, especially with a keyboard shortcut. A confirmation dialog would probably be useful.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @BigLiban, @Darth-Raazi, @Lucifersan, and @saads312)


sinks/dashboard/dashboard.py line 342 at r1 (raw file):

            self.setWindowTitle(f"{title} {unsaved_symbol}")
        elif not changed and unsaved_symbol in title:
            self.setWindowTitle(title[:-2])

what is the point of this? I feel like it would be more robust to just check if the title is equal to the desired title if you are concerned about the performance of changing the window title


sinks/dashboard/dashboard.py line 594 at r1 (raw file):

        self.load()

    def toggle_lock(self):

I don't think it's simpler to merge lock and unlock into one method. Keeping it separate makes it easier to reason about. If you put them together, then it's difficult to keep track of the boolean expressions used


sinks/dashboard/utils.py line 82 at r1 (raw file):

                    self.send_backward.emit()
                case KeyEvent(Qt.Key_R, Qt.ControlModifier):
                    self.remove_all.emit()

I think it's dangerous to have a remove all keyboard shortcut that could be easily pressed by accident

Copy link
Contributor Author

@Darth-Raazi Darth-Raazi left a comment

Choose a reason for hiding this comment

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

I think I fixed most of the issues you mentioned. What exactly do you mean by "it'll follow you around"? Also added a confirm popup for remove all

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @BigLiban, @Lucifersan, @patrick-gu, and @saads312)


sinks/dashboard/dashboard.py line 342 at r1 (raw file):

Previously, patrick-gu (Patrick Gu) wrote…

what is the point of this? I feel like it would be more robust to just check if the title is equal to the desired title if you are concerned about the performance of changing the window title

So the title text only has to be hardcoded in one place. Otherwise if you update the title, you need to change multiple lines of code


sinks/dashboard/dashboard.py line 594 at r1 (raw file):

Previously, patrick-gu (Patrick Gu) wrote…

I don't think it's simpler to merge lock and unlock into one method. Keeping it separate makes it easier to reason about. If you put them together, then it's difficult to keep track of the boolean expressions used

I disagree. Most of the code is the exact same so it makes sense to combine then


sinks/dashboard/utils.py line 82 at r1 (raw file):

Previously, patrick-gu (Patrick Gu) wrote…

I think it's dangerous to have a remove all keyboard shortcut that could be easily pressed by accident

Done.


sinks/dashboard/items/plot_dash_item.py line 180 at r2 (raw file):

    def dynamic_corner_size(self)-> int | float :
        return 0

Removing this since mouse resizing can be disabled globally now

Copy link
Member

@patrick-gu patrick-gu left a comment

Choose a reason for hiding this comment

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

Sorry, I meant if you short click on it, when you move your mouse around it will follow your mouse even when not held down. Thanks for the confirm popup. I think this looks good, except I will turn off mouse resizing by default due to the high volume of complaints.

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @BigLiban, @Lucifersan, and @saads312)


sinks/dashboard/dashboard.py line 342 at r1 (raw file):

Previously, Darth-Raazi (Ozayr Raazi) wrote…

So the title text only has to be hardcoded in one place. Otherwise if you update the title, you need to change multiple lines of code

IMO, the data flow should be one way, from our fields in this class to the window itself. But I won't block on this.

@patrick-gu patrick-gu merged commit 9c26f76 into master Jul 27, 2024
1 of 2 checks passed
@patrick-gu patrick-gu deleted the oraazi/mouse-toggle branch July 27, 2024 03:17
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.

2 participants