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

[preferences] fix the display of file icons #7011

Merged
merged 1 commit into from Feb 5, 2020

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jan 29, 2020

What it does

Fixes #6990
Fixes #7010

  • fixes an issue where the file icon was not properly displayed
  • fixes an issue where the file icon was not properly updated
    based on the current file icon theme. This problem is solved by
    programmatically getting the icon based on the file-icon using
    the labelProvider#getIcon method.

TODO:

  • fix the user file icon not being updated once a file icon theme change event occurs (this is probably due to how the user preferences editor is created).
  • fix the display of file icons in the tab (make it consistent with other tabs) when changing file icon themes.

How to test

  1. start the application and open the preferences widget.
  2. change the file icon theme (the settings tabs (user + workspace)) should properly be updated with the corresponding file icon theme and should also be displayed properly.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added bug bugs found in the application preferences issues related to preferences theming issues related to theming labels Jan 29, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jan 29, 2020
@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Jan 29, 2020

@akosyakov do you know as to why the file-icon for the user settings would not be updated but others will when updating the file-icon theme?

@akosyakov
Copy link
Member

@vince-fugnitto one has to listen for changes on label provider and call getIcon again if a uri is affected

@elaihau
Copy link
Contributor

elaihau commented Feb 3, 2020

I tested in gitpod with both Chrome and Firefox:

File icons across all tabs in the preference editor now look consistent.

Tested all 4 Icon themes, they all look good.

@elaihau
Copy link
Contributor

elaihau commented Feb 3, 2020

One thing needs improvement though:

In a multi-root workspace the icons could look inconsistent.

Screenshot from 2020-02-02 21-24-12

the screenshot was from Chrome.
I understand why it happens - and I am not sure if it is in the scope of this PR.

@akosyakov
Copy link
Member

Code-wise looks good, @elaihau could you finish the review please.

@elaihau
Copy link
Contributor

elaihau commented Feb 3, 2020

Code-wise looks good, @elaihau could you finish the review please.

Sure I will see what Vincent says regarding this comment #7011 (comment)

@vince-fugnitto
Copy link
Member Author

One thing needs improvement though:

In a multi-root workspace the icons could look inconsistent.

Screenshot from 2020-02-02 21-24-12

the screenshot was from Chrome.
I understand why it happens - and I am not sure if it is in the scope of this PR.

Could you help me reproduce the problem you've experienced @elaihau ?

I tried on my end with multiple different file-icon themes and experienced the following:

Theia

Screen Shot 2020-02-03 at 8 30 47 AM

Seti

Screen Shot 2020-02-03 at 8 30 26 AM

Minimal

Screen Shot 2020-02-03 at 8 30 36 AM

None

Screen Shot 2020-02-03 at 8 30 57 AM

@elaihau
Copy link
Contributor

elaihau commented Feb 4, 2020

@vince-fugnitto this is what i see from gitpod. browser is Chrome.

Peek 2020-02-03 20-00

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto this is what i see from gitpod. browser is Chrome.

Thank you! I’ll take a look tomorrow at work :)

@vince-fugnitto
Copy link
Member Author

@elaihau do you expect that the workspace file should display the .json icon?
It is a different type of file (.theia-workspace) and is displayed different in other areas (explorer, file dialog, quick-file-open).

@elaihau
Copy link
Contributor

elaihau commented Feb 4, 2020

@elaihau do you expect that the workspace file should display the .json icon?
It is a different type of file (.theia-workspace) and is displayed different in other areas (explorer, file dialog, quick-file-open).

i don't expect the code to be smart enough to detect json data from a file that doesn't have the ".json" extension.

the question in my mind is: why is the workspace tab header and file icon displayed properly when the multi-root workspace is created, but refreshed into "Untitled.work-space" after the execution of "change file icon theme" command?

like i said earlier, i am not sure if it is in the scope of this PR, and i am OK with not fixing this scenario

@elaihau
Copy link
Contributor

elaihau commented Feb 4, 2020

i will approve this pr after you fix the travis biuld

@vince-fugnitto
Copy link
Member Author

i will approve this pr after you fix the travis biuld

There seems to be an issue with the Windows build at the moment for multiple pull-requests :(
#7064

@vince-fugnitto
Copy link
Member Author

@elaihau do you expect that the workspace file should display the .json icon?
It is a different type of file (.theia-workspace) and is displayed different in other areas (explorer, file dialog, quick-file-open).

i don't expect the code to be smart enough to detect json data from a file that doesn't have the ".json" extension.

the question in my mind is: why is the workspace tab header and file icon displayed properly when the multi-root workspace is created, but refreshed into "Untitled.work-space" after the execution of "change file icon theme" command?

What do you think should be the correct behavior, to display Workspace or the *.theia-workspace name?

@elaihau
Copy link
Contributor

elaihau commented Feb 4, 2020

There seems to be an issue with the Windows build at the moment for multiple pull-requests :(
#7064

yea your latest css change shouldn't have the power of failing the build :)

What do you think should be the correct behavior, to display Workspace or the *.theia-workspace name?

as a user i would expect the tab header text "workspace" stays unchanged.
please double check with others in the team. it is just my opinion.

@vince-fugnitto
Copy link
Member Author

There seems to be an issue with the Windows build at the moment for multiple pull-requests :(
#7064

yea your latest css change shouldn't have the power of failing the build :)

I hope not haha :)

What do you think should be the correct behavior, to display Workspace or the *.theia-workspace name?

as a user i would expect the tab header text "workspace" stays unchanged.
please double check with others in the team. it is just my opinion.

I'll see what I can do :) I believe vscode keeps it as workspace as well.

@vince-fugnitto
Copy link
Member Author

@elaihau it should be fixed in the latest iteration :)

@elaihau
Copy link
Contributor

elaihau commented Feb 4, 2020

@elaihau it should be fixed in the latest iteration :)

yep, i played with it one more time and it looks good.
Thank you for your work !

Fixes #6990
Fixes #7010

- fixes an issue where the file icon was not properly displayed
- fixes an issue where the file icon was not properly updated
based on the current file icon theme. This problem is solved by
programmatically getting the icon based on the `file-icon` using
the `labelProvider#getIcon` method.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto merged commit 44aa52f into master Feb 5, 2020
@vince-fugnitto vince-fugnitto deleted the vf/fix-preferences branch February 5, 2020 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application preferences issues related to preferences theming issues related to theming
Projects
None yet
3 participants