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

Improve UX of drive letters #36069

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Feb 10, 2020

Namely, move the drive dropdown to just the left of the path text box and don't include the former
in the latter.

This improves the UX on Windows.

In the UNIX case, since its concept of drives is (ab)used to provide shortcuts to useful paths, its
dropdown is kept at the original location.

Browsing the file system (Windows)

image
(Note the drive letters at the left of the directory and not being part of it.)

Browsing the file system (Linux)

image
(Note the "drives" at the right and being part of the path. The directory API now reports that drives are shortcuts for UNIX-derived platforms.)


This code is generously donated by IMVU.

@RandomShaper RandomShaper added enhancement topic:editor usability cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 10, 2020
@RandomShaper RandomShaper added this to the 4.0 milestone Feb 10, 2020
@YeldhamDev
Copy link
Member

Why is the "Path:" label all the way to the left in the GNU/Linux screenshot?

@RandomShaper
Copy link
Member Author

Since I did this on an earlier version of Godot, I think this is a rebasing error.

But now I think of it, it may be better to remove the label altogether.

@RandomShaper
Copy link
Member Author

RandomShaper commented Feb 11, 2020

Fixed (and screenshot updated).

Whether to remove labels that add little or no value, like Path or Files & Directories could be studied engine-wide, as a separate concern.

@groud
Copy link
Member

groud commented Feb 11, 2020

UX-wise it looks good to me. I kind of dislike the fact we have to implement a specific set of functions (like get_current_dir_without_drive()) to do that, but if there is no cleaner way then we can go for that.

@RandomShaper
Copy link
Member Author

Yes, I thought about it, but in the end I considered that was the less ugly solution, at least from those I could come up with.

@@ -2367,6 +2367,11 @@ String _Directory::get_current_dir() {
ERR_FAIL_COND_V_MSG(!d, "", "Directory must be opened before use.");
return d->get_current_dir();
}
String _Directory::get_current_dir_without_drive() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could simply add an "include_drive" as an optional argument to get_current_dir(). Those two functions do the same thing in general, so that might be cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

That has a point, but I still prefer a separate method. Boolean parameters are hard to read in the calling location.

Copy link
Member

Choose a reason for hiding this comment

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

I kind of disagree, since most of the time the parameter is not there it does not make a significant difference IMHO. It's better than adding functions with longs name for each possible case.
But well, it's not that important. If others have an opinion on that that would be nice :)

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer optional arguments to multiple functions. I had this discussion earlier in the context of the C# string code. Should be get_current_dir(bool include_drive = true) (or false)

P.S. This needs empty lines as per #33027 "Enforce one line between function implementations"

Copy link
Member Author

Choose a reason for hiding this comment

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

Change done.

Regarding empty lines, such a cosmetic change is something to be done aside of this PR, especially after the requested changes, since now it's not even adding/removing methods at all.

@aaronfranke
Copy link
Member

With the dropdown on the right on Linux, perhaps there should also be a / option? In case users have a drive mounted at e.g. /storage, and they have a project on it.

@RandomShaper
Copy link
Member Author

In that case you probably can just use the go to parent button all the way up.

Or, since you are a power user, able to add mount points that way, you'd probably just type the path without even using the dialog at all. 😆

More seriously now, that's out of the scope of this PR.

@RandomShaper RandomShaper removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 28, 2020
@realkotob
Copy link
Contributor

realkotob commented Feb 28, 2020

Initially this seemed like a good idea but it makes copy-pasting paths much more annoying on windows.

Currently it is possible to quickly interchange between Windows Explorer and the Godot File Browser, as shown below, but will not be possible with this PR, making godot the first PC software to not have this feature.

image

image

@RandomShaper
Copy link
Member Author

You have a point there. I think I can do a couple of modifications to have the best of both worlds.

Namely, move the drive dropdown to just the left of the path text box and don't include the former
in the latter.

This improves the UX on Windows.

In the UNIX case, since its concept of drives is (ab)used to provide shortcuts to useful paths, its
dropdown is kept at the original location.
@akien-mga akien-mga merged commit f83f1d7 into godotengine:master Mar 4, 2020
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the imvu/improve_drives_ux branch March 6, 2020 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants