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

[crop/qt5] spinbox bugfix #212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

szlldm
Copy link
Contributor

@szlldm szlldm commented Jan 19, 2021

-on change set spinbox maximums, to prevent inversion (bad UX)
-these maximums prevent to set output resolution lower than 8x8
-spinboxes get minimum width, to prevent jerkiness (introduced by setMaximum)

@eumagga0x2a
Copy link
Collaborator

I'm sorry for not responding here earlier.

After my struggle with mplayerDelogo which led to 1b7ebce, I fear that Qt doesn't allow setting maximum for a spinbox dynamically while keeping up/down buttons auto-repeat working. Does auto-repeat still work for you in all spinboxes with this change?

I think, minimum width and height should be enforced at parameter validation and other relevant locations in code, also beyond this filter. I just lack a vision how to design this properly at the moment. Last but not least, if a filter may reject input, it must implement a pass-through operation.

@szlldm
Copy link
Contributor Author

szlldm commented Feb 8, 2021

Does auto-repeat still work for you in all spinboxes

Well, it doesn't work here either. I have only tested for up/down arrow keys, those works.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 3, 2023

After my struggle with mplayerDelogo which led to 1b7ebce, I fear that Qt doesn't allow setting maximum for a spinbox dynamically while keeping up/down buttons auto-repeat working. Does auto-repeat still work for you in all spinboxes with this change?

I did a proof-of-concept on this, using PySide6. It seems that auto-repeat is only broken if you update the maximum of the current spinbox, while it's being changed. As long as you're careful to only update other spinboxes' maxima, but leave the current widget alone, auto-repeat works fine.

And that should be sufficient, because changing any spinbox's value can't affect its own maximum value. That should remain static, whatever the current value is set to. Whenever the code was updating maximum values, it would've always been calling setMaximum() on the current widget with the same value it had before. So, the solution is to add logic to... like... not do that. 🤣 If the top value is updating, only call bottom.setMaximum(), as that's the only maximum that could possibly change based on the value of top. If left is changing, only call right.setMaximum(). And so on.

@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 3, 2023

My proof-of-concept, if you want to try it yourself, is in ferdnyc/spinbox_test. Just clone it and run python3 spinbox_test.py from any Python installation/environment that has PySide6 available, and you'll get this dialog:

image

The "Box Totals:" values will be updated based on the spinbox values and maximum values. Auto-repeat on the spinbox arrows works unless "Update all spinboxes" is checked.

@szlldm
Copy link
Contributor Author

szlldm commented Jan 4, 2023

Thanks, will look into that.

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