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 the 'file has changed' dialog #6873

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Improve the 'file has changed' dialog #6873

merged 1 commit into from
Jan 16, 2020

Conversation

kaiyue0329
Copy link
Contributor

Issue: #5831

What it does

  • Improve the "file has been changed" dialog

How to test

  • Need help with steps to reproduce.

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The dialog now correctly displays the shorter title with only the filename:

image

Do you think it'd be good to display the full path using labelProvider.getLongName within the actual body of the dialog? This will avoid confusion for users who may have multiple files with the same name in their workspace with different paths, and help them quickly identify the file in question.


Something like:

msg: `Do you want to overwrite the changes made to "${labelProvider.getLongName(new URI(file.uri)}" on the file system?`,

@lmcbout
Copy link
Contributor

lmcbout commented Jan 13, 2020

The path would remove ambiguity. Can you add or display it as a tooltip over the dialog ?

@vince-fugnitto
Copy link
Member

The path would remove ambiguity. Can you add or display it as a tooltip over the dialog ?

I think a tooltip would be too complicated and un-intuitive for a pop up dialog.
I propose to add the getLongName() directly in the body of the dialog as I previously commented.

@akosyakov akosyakov added the filesystem issues related to the filesystem label Jan 14, 2020
@vince-fugnitto
Copy link
Member

@kaiyue0329 do you mind squashing your commits into a single commit, preserving the detailed commit message?

- Use LabelProvider.getName(uri) to get a systemwide human readable representation of a simple file name.
- Use LabelProvider.getLongName(uri) to get a systemwide human readable representation of a full path.

Signed-off-by: Kaiyue Pan <kaiyue.pan@ericsson.com>
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

It looks fine to me 👍
I won't close #5831 since it describes adding new buttons, but in my opinion they aren't explicitly useful.

Screen Shot 2020-01-14 at 1 51 29 PM

@lmcbout
Copy link
Contributor

lmcbout commented Jan 14, 2020

Looking at the picture, can you make the window smaller by putting the message data in two lines , i think it would be better since the file path is very long

@vince-fugnitto
Copy link
Member

Looking at the picture, can you make the window smaller by putting the message data in two lines , i think it would be better since the file path is very long

@lmcbout not sure the change should be applied here. Would it not be better to have a max-width for dialogs in general? This means that the application will be responsible for updating the size and not extenders.

Example, I created the following dialog:

Screen Shot 2020-01-14 at 2 25 52 PM

I think this is a separate enhancement that can be tracked in an issue.

@lmcbout
Copy link
Contributor

lmcbout commented Jan 14, 2020

Very very long text could be handle on a separate issue, It is OK for me
Looks good

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

LGTM

@vince-fugnitto
Copy link
Member

@kaiyue0329 I think you might have pushed changes unrelated to this pull request instead of opening a new one.

@kaiyue0329
Copy link
Contributor Author

@vince-fugnitto I will revert the unrelated changes and create a new branch.

@vince-fugnitto vince-fugnitto merged commit 135af5d into eclipse-theia:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants