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

ENH: Improve usability of Directory Button by using native dialog #973

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

jamesobutler
Copy link
Contributor

This aims to improve directory selection by using the native dialog instead of Qt's own widget-based dialog design.

See 3D Slicer forum discussion of how users have been doing other methods to avoid using widgets that use Qt's widget-based dialog.
https://discourse.slicer.org/t/using-native-file-dialog-for-selecting-files-and-folders/17926

Qt widget-based dialog Native dialog (on Windows)
image image

@lassoan
Copy link
Member

lassoan commented Jun 4, 2021

Thank you for working on this. Native file dialogs are now probably preferred by most users, so I think it is a good idea to use them by default.

Is there a reason why you switched from ctkFileDialog to QFileDialog instead of just using the DontUseNativeDialog flag to choose between native and CTK style? You could even change the default to use native dialog, but keep the door open for using the CTK style dialog to reduce the potential undesired impact of the change.

@jamesobutler
Copy link
Contributor Author

I switched from ctkFileDialog to QFileDialog because the customizations of ctkFileDialog appeared to be mostly related to modifying the functionality of a Qt widget-based dialog. So switching to the native file dialog is essentially going back to using a QFileDialog without those additional customizations.

We can't just remove the following DontUseNativeDialog specification as there is other code in the class that requires the use of a Qt widget-based dialog with calls such as setBottomWidget.

// The findChild<QDialogButtonBox*>() call fails on Mac/Qt5 because native
// dialogs don't publish any internals. No problems on other OS.
// Can be applied to Qt4 as well, if problems arise there.
#if QT_VERSION >= QT_VERSION_CHECK(5,0,0)
this->setOptions(DontUseNativeDialog);
#endif

@lassoan
Copy link
Member

lassoan commented Jun 4, 2021

The extra features don't have to be deleted, but they would only be used if dontusenativedialog flag is enabled.

@lassoan
Copy link
Member

lassoan commented Jun 5, 2021

Thanks for the updates. These small changes are much less likely to cause any regressions in any CTK-based projects. Just need to sort out those few syntax errors.

@jamesobutler jamesobutler force-pushed the native-file-dialogs branch from b777d11 to e96dc1d Compare June 5, 2021 20:55
Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
@jamesobutler jamesobutler force-pushed the native-file-dialogs branch from e96dc1d to 10824ae Compare June 7, 2021 20:05
@jamesobutler
Copy link
Contributor Author

I've pushed an update based on @finetjul 's suggestions

@jamesobutler jamesobutler requested review from finetjul and lassoan June 7, 2021 20:06
@jamesobutler
Copy link
Contributor Author

Thoughts on latest updates @lassoan @finetjul ?

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you!
I had just two comments that should be easy to address.

Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
@jamesobutler jamesobutler force-pushed the native-file-dialogs branch from 10824ae to 858ed65 Compare June 10, 2021 16:03
Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good to me.

@jamesobutler jamesobutler merged commit c4f1eee into commontk:master Jun 10, 2021
@jamesobutler jamesobutler deleted the native-file-dialogs branch June 10, 2021 19:07
SIGNAL(selectionChanged(QItemSelection,QItemSelection)),
q, SLOT(onSelectionChanged()));
this->UsingNativeDialog = !(q->options() & QFileDialog::DontUseNativeDialog);
if (!(this->UsingNativeDialog))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!(this->UsingNativeDialog))
if (!this->UsingNativeDialog)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed commit with change to #977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants