Why did Add Existing Files Dialog UI change?

[chocoberry317]

Hi. I recently updated Sigil to 2.6.0 and noticed that the Add existing files dialog UI has changed.
But, I didn't find anything related to this in the changelog of Github. I wonder why the UI has changed. The old UI was intuitive and also convenient because it was possible to move by directly entering the path of the folder where the files to add were located. Is it impossible to use the old UI again other than going back to 2.5.2? It's a shame.

[DNSB]

You may want to check the Sigil crashes when loading file from the internet thread or message #11 in the Sigil-2.6.0 Released thread for the reasons why Sigil is now using the non-native file dialog for Windows.

[BeckyEbook]

I admit that I also like the native system window, so I played around with simple filtering and in Windows it works.

I know that `QFileDialog::DontUseNativeDialog` is safer, but let's be honest: how many people are going to type in the graphics link instead of pointing to the appropriate file from disk?

Such code works for me.
So I leave the condition only Q_OS_MAC and check the beginning of the path. If http/https then I display a message, but with no crash.

    QFileDialog::Options options = QFileDialog::Options();
#if defined(Q_OS_MAC)
    options = options | QFileDialog::DontUseNativeDialog;
#endif

    // tempfilepaths are full absolute file paths to the files to be added
    QStringList tempfilepaths = QFileDialog::getOpenFileNames(this,
        tr("Add Existing Files"),
        m_LastFolderOpen,
        filter_string,
        NULL,
        options);

    QStringList filepaths;
    for (const QString &path : tempfilepaths) {
    if (path.startsWith("http://", Qt::CaseInsensitive)
     || path.startsWith("https://", Qt::CaseInsensitive)) {
        Utility::DisplayStdErrorDialog(
            tr("The path \"%1\" is not a local file and was not added.").arg(path));
        continue;
    }
        filepaths << path;
    }

Such a preliminary check of the existence of links should be enough, and Windows users will not turn their noses up at the default Qt window.

[DiapDealer]

Originally Posted by chocoberry317

But, I didn't find anything related to this in the changelog of Github.

Really? It's the very first bugfix item mentioned in the 2.6.0 changelog and in the 2.6.0 release notes.

[KevinH]

I am confused. What really is the issue here? The Qt non-native can show icons as well (see the icon icon at the top right), and can store any opened path/folder on the left to speed selections up. And it will remember them.

What does the native dialog do that the non-native dialog cannot that is upsetting people when selecting local files? Is it just people against change (even though it prevents a crash) or something tangible?

[DiapDealer]

Originally Posted by KevinH

Is it just people against change (even though it prevents a crash) or something tangible?

I feared this kind of reaction in the back of my mind, but yes... when people mention things like "unintuitive" in their reasoning, I find that it usually translates to "I'll have to adjust my process", rather than something actually being unintuitive. Had Sigil not used native dialogs on Windows all along, we'd not be having a discussion about intuitiveness. *shrug*

I'm not exactly opposed to offering people a way out of this recent change, but I'm not really sure it's in their best interest in the long run, either. Because I can see an eventual transition to non-native dialogs everywhere in Sigil (for all platforms). The amount of code we maintain that determines how dialogs are launched on the various supported platforms (not to mention code needed to prevent platform specific crashes) is already clunky and hard to maintain. Non-native just tends to work everywhere without barfing.

If we allow a way for individual platforms to override non-native dialogs in that potential non-native-for-all future edition of Sigil, we'd need a utility function of some kind that would that handle (in a cross platform manner) whether a native or non-native dialog was required. That function could then be used everywhere a dialog is called for.

I'm afraid I'm not interested in coding (or maintaining) any more piecemeal stuff for dialog launches scattered throughout Sigil's codebase.

[KevinH]

Something like a Utility.cpp static routine to OR in the proper non-native flag or native flag when passed in options so it can be inlined right inside the call to the filedialog routine.

Then we could remove all ifdefs for platform from all filedialogs and replace the options value with a call to this utility routine.

In Utility.cpp we could check for a SIGIL_FORCE_NATIVE_FILE_DIALOG env variable and if set use the native for all platforms or put ifdefs for platforms all there.

But again, if the user sets that env var it is set at there own risk and no crashes caused by native filedialogs would be investigated anymore.

It is important to note Qt has a really lousy track record when trying to support native file dialogs as there have been repeated crashing bugs reported there with only temp patches and fixes added. So many developers that only target one platform must want native filedialogs and native dialogs in general, but for a cross platform app in a day and age where people are familiar and work with multiple os versions and web apps, it does seem strange. God forbid Windows starts injecting ads into their native dialogs!

Is something along those lines what you are thinking?

[DiapDealer]

Originally Posted by KevinH

Is something along those lines what you are thinking?

Very much so.

I think we would probably need to maintain ifdefs for each platform in the utility function, rather than setting things for all platforms with one variable. I believe there are many places where native dialogs simply won't work on macOS. Specifically where zip files need to be selected by a dialog rather than browsed into. Or am I remembering something else in that regard?

Non-native dialogs may break some theming stuff on Linux, so I'd need to do some checking before determining if non-native dialogs on Linux should be the default, or the exception.

[KevinH]

Here is what I think will get us close to what we have now in the code with a env var override.

It needs to be called to replace QFileDialog::Options options = QFileDialog::Options().

The issue is we may want to invoke this only on some routines and not others so more work will be need to handle special cases:

//declaration for header
QFileDialog::Options DlgOptions(const QString special_case = QString());


// to be used in place of a call to QFileDialog::Options()                                                                         
QFileDialog::Options Utility::DlgOptions(const QString special_case)
    QFileDialog::Options options = QFileDialog::Options();
    if (qEnvironmentVariableIsSet("SIGIL_FORCE_NATIVE_FILE_DIALOG")) {
      	return options;
    }
#ifdefined(Q_OS_MAC)
    options = options | QFileDialog::DontUseNativeDialog;
#endif
#ifdefined(Q_OS_WIN32)
    if (!special_case.isEmpty()) {
        options = options | QFileDialog::DontUseNativeDialog;
    }
#endif
    return options;
}

We could pass in other options as a parameter but it may be easier to add those other options in as needed back in the original places we use them now (things like QFileDialog::HideNameFilterDetails, and QFileDialog::ReadOnly and any others we use in various places.

There really are a lot of them.


Thoughts?

[KevinH]

@DiapDealer,

I just pushed a modified version of that routine plus all the other changes to centralize initialization of QFileDialog::Options into Utility::DlgOptions() all throughout the Sigil code.

I added in the env var override but we could remove it easily enough if we do not decide to go that way.

I do think with this new approach everything is centralized making it easier to make global platform changes later and it lessens the need for ifdefs everywhere throughoput the code that invoked a QFileDialog while adding just a few to Utility::DlgOptions(). It really removed a lot of Q_OS_MAC ifdefs.

Note: Utility::DlgOptions() has an added parameter special_case (defaults to an empty string) that should be flexible enough to allow us to make exceptions in any place we will need to in the future. I use it now just to match exactly what we had been doing before this change.

Please give it a look see and let me know if you want me to revert any or all of it.

Hope this helps!

[DiapDealer]

It's sounds great! I'll take a look a bit later. Thanks!

[DNSB]

I compiled Sigil with the changes and played with the file dialog. I could toggle the native and non-native dialogs and seemed to be no issue with either. Oddly, I found myself preferring the non-native dialog since I've added my favourite directories to the left column along with a few other items and have had a while to get used to it under Windows. This may not matter to many but I also like the consistency when playing with Sigil on Windows, MacOS and Linux.

[KevinH]

Glad to hear it is working! Thanks for building and testing it.

[DiapDealer]

I found myself a little taken aback when the native dialog reappeared after recompiling and setting the env var. After getting used to the non-native one, it seemed unnecessarily huge!

[DNSB]

How soon we forget...