Skip to content

Conversation

@gonX
Copy link
Member

@gonX gonX commented Jan 8, 2025

Fixes #3668 for 0.6.x

Previously, we assumed that Eto.EtoEnvironment.GetFolderPath would never return an empty string. It is however apparent that this is actually the case on some systems, seen on both Windows and Linux.

Constructing URI's with an empty string throws an exception, which this commit attempts to avoid. It was not an option to simply wrap the Uri constructor, as Directory in SaveFileDialog/OpenFileDialog must only be set if it is a valid Uri, so instead the SaveFileDialog/OpenFileDialog Eto constructor has been moved to a wrapper method internal to the project.

Previously, we assumed that Eto.EtoEnvironment.GetFolderPath would never
return an empty string. It is however apparent that this is actually the
case on some systems, seen on both Windows and Linux.

Constructing URI's with an empty string throws an exception, which this
commit attempts to avoid. It was not an option to simply change the Uri
constructor, as Directory can only be set if it is a valid Uri, so
instead the Save/OpenFileDialog Eto constructor has been moved to an
extension method internal to the project.
@gonX gonX added bug Something isn't working gui Affects driver GUI labels Jan 8, 2025
@gonX gonX added this to the v0.6.5.1 milestone Jan 8, 2025
@gonX gonX added the needs-forward-port PR or its features needs to be ported to development branch label Jan 8, 2025
@Kuuuube
Copy link
Member

Kuuuube commented Jan 16, 2025

Looks good. Has this been tested by users that had this problem?

@Mrcubix
Copy link
Contributor

Mrcubix commented Jan 16, 2025

Seems like you could jus re-use OpenFileDialog for SaveFileDialog provided you offer another param for the title
& either making it generic or another param for whether it's for saving or not

public static FileDialog OpenFileDialog(string title, string directory, FileFilter[] filters, string defaultTitle, bool multiSelect = false, bool saving = false)
{
    var fileDialog = saving ? new SaveFileDialog() : new OpenFileDialog();

    SetupFileDialog(fileDialog, title, directory, filters, defaultTitle);
    fileDialog.MultiSelect = multiSelect;

    return fileDialog;
}

Also, i don't see the point of another title parameter if it's set everywhere.

public static void AddFilterRange(this FileDialog fileDialog, FileFilter[] filters)
{
    if (filters == null)
        foreach (var filter in filters)
            fileDialog.Filters.Add(filter);
}

//public static FileDialog OpenFileDialog<T>(string title, string directory, FileFilter[] filters, bool multiSelect = false)
//where T : FileDialog, new()
public static FileDialog OpenFileDialog(string title, string directory, FileFilter[] filters, bool multiSelect = false, bool saving = false)
{
    // var fileDialog = new T();
    var fileDialog = saving ? new SaveFileDialog() : new OpenFileDialog();

    fileDialog.AddFilterRange(filters);

    if (!string.IsNullOrEmpty(title))
        fileDialog.Title = title;

    if (!string.IsNullOrEmpty(directory))
        fileDialog.Directory = new Uri(directory);

    fileDialog.MultiSelect = multiSelect;

    return fileDialog;
}

also where is the actual opening part? return a bool instead by calling ShowDialog with a parent arg.

@gonX gonX changed the title UX: Rewrite FileDialog directory checks [06x] UX: Rewrite FileDialog directory checks Jan 16, 2025
@gonX
Copy link
Member Author

gonX commented Jan 16, 2025

Seems like you could jus re-use OpenFileDialog for SaveFileDialog provided you offer another param for the title
& either making it generic or another param for whether it's for saving or not

Not sure if I follow. I still want to return the correct type - is this possible without making a template class? Your changes return a SaveFileDialog type on a method that should return a OpenFileDialog return type, which won't work as-is with the provided diff. And yes, setting title can obviously be merged if you can merge the open/save filedialog functions in front.

@Mrcubix
Copy link
Contributor

Mrcubix commented Jan 17, 2025

Both inherit from FileDialog which inherit from Dialog

@gonX
Copy link
Member Author

gonX commented Jan 18, 2025

Looks good. Has this been tested by users that had this problem?

Has been tested now: discord verification

Will refactor this to use a template as discussed in DM's with Cubix.

@gonX gonX marked this pull request as draft January 18, 2025 21:33
@gonX gonX self-assigned this Jan 18, 2025
@gonX gonX marked this pull request as ready for review January 22, 2025 12:01
Copy link
Member

@jamesbt365 jamesbt365 left a comment

Choose a reason for hiding this comment

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

Approving with good faith that this works, code looks fine at a glance.

@gonX
Copy link
Member Author

gonX commented Jan 24, 2025

Pretty sure we should be good to merge, absolutely worst case it's buggy and it'll be fixed/reverted before release anyway.

@gonX gonX merged commit 9130e9b into OpenTabletDriver:0.6.x Jan 24, 2025
8 checks passed
@gonX gonX deleted the 06x-dont-use-empty-string-directories branch January 24, 2025 02:30
@gonX gonX removed the needs-forward-port PR or its features needs to be ported to development branch label Feb 3, 2025
@gonX gonX removed their assignment Feb 3, 2025
@gonX
Copy link
Member Author

gonX commented Feb 3, 2025

Removing forward port tag because master is not taking code changes

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

Labels

bug Something isn't working gui Affects driver GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eto special directories ("my documents" et.al.) sometimes return empty strings

4 participants