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

Override cursor handling in spinbox and autocommit is buggy ... #160

Open
markotoplak opened this issue Jul 1, 2021 · 2 comments
Open

Comments

@markotoplak
Copy link
Member

... at least in theory: I did not try to reproduce the bug, I just see it in code. We fall into the same trap as this guy: https://stackoverflow.com/questions/66185831/pyqt5-how-to-restore-the-default-cursor-after-multiple-overrides

Whenever we override an application-wide cursor, these go onto a stack. For example, if anything overrides the cursor while you are dragging in the spinbox, then after drag the overridden cursor will not be restored.

I do not see a safe use of override cursors. As a hack, restores could become something like the following, which guarantees that the default cursor will always be restored.

while QApplication.overrideCursor() is not None:
   QApplication.restoreOverrideCursor()

To use it, we would also need to be sure that QApplication.restoreOverrideCursor() does crash if the current stack is empty (because this code could clean someone else's cursor).

So I'd avoid override cursors completely. SpinBoxes could probably be rewritten to capture the mouse and then use .setCursor on themselves.

@irgolic
Copy link
Member

irgolic commented Jul 1, 2021

while QApplication.overrideCursor() is not None:
   QApplication.restoreOverrideCursor()

Sure, we can do that. In the rare possibility that we write cursor overrides that trigger while another widget is focused, that will save us.

So I'd avoid override cursors completely.

They're very valuable from a UX perspective, and with the fix you presented, I don't see what threat they pose.

SpinBoxes could probably be rewritten to capture the mouse and then use .setCursor on themselves.

You're right, that would be a great solution. The behavior now works fine, but if the spinbox were to capture the cursor and not let it escape, the drag functionality wouldn't be limited to just your screen size. The cursor should still be overridden.

irgolic added a commit to irgolic/orange-widget-base that referenced this issue Jul 1, 2021
irgolic added a commit to irgolic/orange-widget-base that referenced this issue Jul 1, 2021
@markotoplak
Copy link
Member Author

Sure, we can do that. In the rare possibility that we write cursor overrides that trigger while another widget is focused, that will > They're very valuable from a UX perspective, and with the fix you presented, I don't see what threat they pose.

I wrote sloppily: I do agree that cursor changes are useful, therefore we should make sure they are correct. Wherever I see QApplication.setWhatever I think of problems. We can never know what code runs in the meanwhile. I'd avoid it if there is another way.

That spinbox is the smaller problem though: the autocommit is worse. :) Just writing the issue here so that we become more aware of similar issues...

irgolic added a commit to irgolic/orange-widget-base that referenced this issue Jul 11, 2021
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

No branches or pull requests

2 participants