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

Configurable window positioning behavior after matching image size #616

Closed
wants to merge 1 commit into from

Conversation

jdpurcell
Copy link
Contributor

Implements #487.

@@ -64,6 +64,7 @@ QVOptionsDialog::QVOptionsDialog(QWidget *parent) :
#endif

syncSettings(false, true);
connect(ui->windowResizeComboBox, QOverload<int>::of(&QComboBox::currentIndexChanged), this, &QVOptionsDialog::windowResizeComboBoxCurrentIndexChanged);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed a bug in the existing code where this never got connected up.

@jurplel
Copy link
Owner

jurplel commented Aug 27, 2023

My main concern with this is the UX. It's a pretty confusing option. More direct language might help--but after revisiting this issue I am questioning if anyone is actually going to use this option. Unfortunately I don't remember my original motivation for the issue, but maybe this needs a rethink.

If we have an options cleanup with more tabs and stuff, this option should/could certainly exist, but right now the settings dialog is getting cluttered.

@jdpurcell
Copy link
Contributor Author

@jurplel No worries, if you want we could close this for now and I could make a new pull request if you like the part with the windowResizeComboBox code deduplication and currentIndexChanged wire-up.

@jurplel
Copy link
Owner

jurplel commented Aug 27, 2023

Yes, I think that'd be great for now. Thanks!

@jurplel jurplel closed this Aug 27, 2023
@jdpurcell jdpurcell deleted the aftermatchingsize branch September 26, 2023 13:44
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