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

Change icon folder to custom icon #1189

Closed
wants to merge 32 commits into from
Closed

Change icon folder to custom icon #1189

wants to merge 32 commits into from

Conversation

alonfnt
Copy link

@alonfnt alonfnt commented Jan 29, 2020

In order to learn a bit how this repo works, I've tried to fix #171, but I need some guidance to continue, so don't take this PR too serious.

My current state:

  • Change Icon Image to Toggle button
  • Use theme icons name to change icon
  • Make the change display in the directory view
  • Save custom icon name so that it stays after refreshing directory

This gif demostrates the current state of the PR ( I know using the firefox exemple is a bad one, but it was just as a demostration )
change_icon_PR

The icon is changed when dragged or when the properties dialog is opened again though.

Ideally we would be able to put custom images as icons, but this is just a first attempt.

If this pull request is wrong feel free to close it.

@alonfnt
Copy link
Author

alonfnt commented Jan 29, 2020

Update: I have added the possibility to use images as icons. The two main issue are still present though. No update in the directory view and no persistence. Will keep working on them.

update_pr_custom_icons

@@ -0,0 +1,102 @@
/*
* Copyright (c) 2011 Marlin Developers (http://launchpad.net/marlin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not need this line - this is a new class

@@ -0,0 +1,102 @@
/*
* Copyright (c) 2011 Marlin Developers (http://launchpad.net/marlin)
* Copyright (c) 2015-2018 elementary LLC <https://elementary.io>
Copy link
Collaborator

Choose a reason for hiding this comment

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

New code should be Copyright 2020 elementary, Inc. (https://elementary.io) now

@jeremypw
Copy link
Collaborator

@AlbertAlonso Glad to see a new contributor to Files - you have dived in the deep end!

I have made a couple of preliminary comments on the code.

If you install io.elementary.vala-lint (https://github.com/vala-lang/vala-lint) you can run your PR commits against it before pushing to avoid CI lint failures. Easy to forget to do it though.

Best get comment from UX team before going too far to check they are happy going down this route and what constraints should be applied. Another issue is how are you going to persist the custom icon per folder?

@jeremypw jeremypw requested a review from danirabbit January 29, 2020 16:19
@alonfnt
Copy link
Author

alonfnt commented Jan 29, 2020

@jeremypw Thanks for the comments!

Since I started doing it just to get practice, I didn't really thought about if it was right or wrong, so definitely good call on asking the ux team before continuing.

TBH I don't really know how to get persistence. I guess I'll just keep playing with it until something works or someone points me to the right direction!

PS: The vala-lint is gonna save me from so many fix-commits you can not imagine! Thank you!

@arkraft
Copy link

arkraft commented Jan 30, 2020

I also started with the task, but from the other side, i first wanted to display the icon and then think about how to solve the UI. I looked up how Nautilus solves the problem. The use GIO and custom metadata which you can set on folders. This can then be read using FileInfo to display the icon. This is a snippet of how i did it (File.vala inside the method get_folder_icon_from_uri_or_path):

FileInfo info = location.query_info ("metadata::custom-icon-name", 0);
if (info != null) {
    custom_icon_name = info.get_attribute_as_string ("metadata::custom-icon-name");
    if (custom_icon_name != null) {
	icon = new GLib.ThemedIcon (custom_icon_name);
    }
}

There is a constant GIO_DEFAULT_ATTRIBUTES, i also added metadata::custom-icon-name there but i don't know if this is required or optional.

It works great. Only thing i did not test is how to set it usind GIO but i think it's pretty straightforward like reading it. I used the CLI to set it for testing

@jeremypw
Copy link
Collaborator

For persistence, I can think of two possible approaches - using file meta-data (like sort order) or using the same approach as the color-tag plugin which is to maintain a database. Although, to be honest I have considered whether meta-data could be used for color-tags as well as it is more lightweight.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 2, 2020

Another thing to consider is whether it is a good idea to allow the custom icon set in a desktop file to be overridden - allowing one executable to "masquerade" as a different one. Could this be a security issue? Maybe custom icons should be limited to folders?

@alonfnt
Copy link
Author

alonfnt commented Feb 7, 2020

UPDATE:

I've tried to fix a bit the Icon selector since it was too easy to mess with the icon and break the icon.
This is also an update on the issues facing this PR and its current state.

Current State:

  • Only Directories can be changed icon (Security reasons)
  • Custom icon is stored in metadata
  • The icon is changed correctly.
  • The ListBox doesn't allow to set invalid Icon themed names

Current Issues:

  • After changing the image, the directory page has to be reloaded in order to see the change. If the previous icon is also custom, you have to refresh twice.
  • To update the icon change in the sidebar, you have to refresh twice.
  • If a custom .png file is used, the sidebar icon is not displayed.
  • If the custom image is removed or renamed, the icon is lost, so maybe a hidden storage somewhere is needed?

Here is the gif showing the current state for those who can't test it.

custom_folder_PR

This is the final result and the reason I started this PR.
Final_result_pr

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 8, 2020

I am wondering whether it would be better for the custom icon to be superimposed on the standard folder icon so that it remains clear that it is a folder. However, that would require messing with the icon renderer. Need UX team input.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 8, 2020

I think we need a convenient way to revert the custom icon to the default (i.e. remove the metadata). At the moment it looks like you have to set the default folder icon as a custom icon??

@alonfnt
Copy link
Author

alonfnt commented Feb 8, 2020

@jeremypw Thanks for the help!

I am wondering whether it would be better for the custom icon to be superimposed on the standard folder icon so that it remains clear that it is a folder. However, that would require messing with the icon renderer. Need UX team input.

That was the original idea and I would love to do that, however it doesn't seem trivial to implement, more so considering how little do I know about Vala and files.

I think we need a convenient way to revert the custom icon to the default (i.e. remove the metadata). At the moment it looks like you have to set the default folder icon as a custom icon??

I tried to have a Reset button that removed the attribute with remove attribute but it didn't seem to work.
Then I set that an empty custom-icon-name string meant not having a custom icon, however with the "redisign" of the popup I didn't know where to put the button. But yes, I also think there should be one. Maybe if the file has metadata, change the popup to a simpler one with just a Reset button? I hope we heard from the UX team soon.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 8, 2020

I'll see if I can come up with something. There also seems to be a bug if you choose "Browse" instead of choosing from the list - the custom icon is not set. Not sure how to control which icons are chosen if we allow browse anyway. Another issue is zooming a custom icon does not work at the moment. Maybe we should require custom icons to be scalable.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 9, 2020

One issue that has arisen is that only some icons are compatible with the compositing process - others give visual glitches - the reason is not yet clear.

@jeremypw
Copy link
Collaborator

Examples of embedded icons, one ok, one with visual glitches.

Screenshot from 2020-02-10 15 47 58

@jeremypw
Copy link
Collaborator

Current appearance of Properties dialog for customised folder with an emblem:
Screenshot from 2020-02-10 15 50 34

Current appearance of IconPopover with customised icon and emblem
Screenshot from 2020-02-10 15 55 21
and Use Default button:

@jeremypw
Copy link
Collaborator

Moving and copying a customised folder also moves/copies the custom icon now, at least within the same filesystem.

@alonfnt
Copy link
Author

alonfnt commented Feb 17, 2020

When you are in List view for instance and you make the icons smaller, All the icons go to lose the folder icon except the custom icons
Screenshot from 2020-02-17 10-21-36

Should we change it to the icon itself for 24px and 16px?

@jeremypw
Copy link
Collaborator

jeremypw commented Mar 2, 2020

@AlbertAlonso In the illustration you are in the home directory where the special user folders have "special" icons - these change for small icons even in master so I do not think it is an issue we should address here. Maybe we should stop system special folders being customised by the user though.

I am loath to spend too much time on this before we get a review from the UX team to avoid going too far down the wrong road.

@danirabbit danirabbit added the Needs Design Waiting for input from the UX team label Jul 21, 2020
@hanaral
Copy link

hanaral commented Jan 29, 2021

OK, so how about using a small collection of different symbolic icons? That way there is not way for the user themselves to do anything that hasn't been tested. I really dislike the idea of allowing users to completely steamroll any semblence of icon consistency and testability.

Edit: How about using the applications-* icons? applications-development, applications-education, applications-games, applications-graphics, applications-multimedia, applications-office and applications-science go really well IMO.

@jeremypw
Copy link
Collaborator

@hanaral That seems sensible and doable, but this PR needs input from the UX team before pursuing.

@danirabbit
Copy link
Member

I think we still need to see a rational of "why". People keep getting caught up in the "how" without asking what problem is being solved.

@jeremypw
Copy link
Collaborator

I guess it is to give an "at a glance" indication of the type folder contents without having to read the name (which might be in small text) 🤷

@hanaral
Copy link

hanaral commented Jan 29, 2021

I still don't particularly like user interference with iconography, so making the folder colour tags less fugly might incentivise using them.

@jeremypw
Copy link
Collaborator

I am inclined to close this PR as it does not look as if it is likely to get any traction.

@alonfnt
Copy link
Author

alonfnt commented Jan 30, 2021

I am inclined to close this PR as it does not look as if it is likely to get any traction.

I agree with you :)

@jeremypw jeremypw closed this Jan 30, 2021
@ChildishGiant
Copy link

So this feature is confirmed not coming? This is supported by Windows, Mac and Ubuntu and helps quickly identify folders at a glance. I feel like it'd be a shame to drop all this work done on the currently most requested feature for files

@jeremypw
Copy link
Collaborator

The UX team need to be persuaded as to what the problem is and what is the best way to solve it. The best way would be to open a well-thought out issue report where a solution design can be developed and gain UX approval. This PR was probably premature.

@nakotami
Copy link

nakotami commented Jan 6, 2022

I feel like we should take another look at this.

There are already folder icons for videos, pictures, etc. Users should be able to add their own custom folders and apply icons to them so they don't stick out like a sore thumb.

@jeremypw
Copy link
Collaborator

jeremypw commented Apr 8, 2022

Please make any further comments on the related issue report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Waiting for input from the UX team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support changing folder icons
7 participants