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

Check for get_icons().props.names absence #69

Closed
wants to merge 3 commits into from

Conversation

tchx84
Copy link
Member

@tchx84 tchx84 commented Jul 28, 2013

For some unknown reason, some volumes does not
have names property. This cause Journal and Volumes
device icon to break.

Signed-off-by: Martin Abente Lahaye tch@sugarlabs.org

For some unknown reason, some volumes does not
have names property. This cause Journal and Volumes
device icon to break.

Signed-off-by: Martin Abente Lahaye <tch@sugarlabs.org>
@dnarvaez
Copy link
Contributor

It would be good to know which kind of object icon is. Even if it's not a themed icon it might be an icon we can display...

@dnarvaez
Copy link
Contributor

Might be a GFileIcon for example and we can display that.

@tchx84
Copy link
Member Author

tchx84 commented Jul 28, 2013

What do you suggest? When the first check fails, test for props.file [1]? and then using that one?

Refs:

  1. https://developer.gnome.org/gio/2.36/GFileIcon.html

@dnarvaez
Copy link
Contributor

I'm more suggesting that we need to know more about what is going to come up with a complete fix. What about changing your patch to do something like

icon = self._mount.get_icon()
...
logging.error('Cannot find icon names for %s, %s', icon, str(mount))

And ask Suraj to try it out and post his shell.log. That will tell us the type of icon we are dealing with.

@tchx84
Copy link
Member Author

tchx84 commented Jul 28, 2013

Sure, will do.

Signed-off-by: Martin Abente Lahaye <tch@sugarlabs.org>
Affects also volumes toolbar icons. We need to
figure out where to put this _find_icon_name
method, so we do not repeat all over.

Signed-off-by: Martin Abente Lahaye <tch@sugarlabs.org>
@tchx84
Copy link
Member Author

tchx84 commented Jul 28, 2013

Whatever we do, we should put it in one place only. So we don't repeat code all over.

@dnarvaez
Copy link
Contributor

I think we should check the icon type using isinstance() and support both GThemedIcon and GFileIcon. It make sense to fallback to a generic icon like you did in your patch as a fallback, if we get a type we don't know about. If supporting GFileIcon is hard I'm fine with just logging an error as a first step.

@tchx84
Copy link
Member Author

tchx84 commented Jul 28, 2013

I will try to get a cleaner version of the current patch first (using isinstance). Any suggestion on where I could put this _find_icon_name method? Id hate to have 3 copies on this same method.

Regarding using GFileIcon, I need more information about it and determine how much it would affect other parts.

@tchx84 tchx84 closed this Jul 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants