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

PR: Add a way to handle font errors in QtAwesome #3886

Merged
merged 7 commits into from
Jan 3, 2017

Conversation

dalthviz
Copy link
Member

Fixes #3843


Is important to know that for this to actually work this PR has to get a merge in the qtawesome project because of the FontError import.

@dalthviz
Copy link
Member Author

A preview of the message dialog:

corruptfont

@dalthviz dalthviz changed the title PR: Added a message dialog to inform the way to handle corrupted font files. PR: Add a message dialog to inform the way to handle corrupted font files. Dec 27, 2016
@dalthviz dalthviz changed the title PR: Add a message dialog to inform the way to handle corrupted font files. PR: Add a message dialog to inform the way to handle untrusted/corrupted font files. Dec 28, 2016
except FontError as fontError:
import traceback
traceback.print_exc(file=STDERR)
traceback.print_exc(file=open('spyder_crash.log', 'w'))
Copy link
Member

Choose a reason for hiding this comment

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

We don't need these three lines.

" eventually creating a new issue <a href=\"%s\">here</a>. "
"Your feedback will always be greatly appreciated."
"" % (fontError ,__project_fonts__, __project_url__,
__forum_url__, __project_url__))
Copy link
Member

Choose a reason for hiding this comment

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

Please rephrase this text like this

Spyder can't load the <i>Spyder 3</i> icon theme. That's why it's going to
fallback to the theme used in Spyder 2.

For that, please close this window and start Spyder again.

and make it translatable.

Copy link
Member

Choose a reason for hiding this comment

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

And after this dialog, we need to add this line

CONF.set('main', 'icon_theme', 'spyder 2')

(but please test that my approach works correctly by removing the QtAwesome fonts in your computer and starting Spyder again).

Copy link
Member

@ccordoba12 ccordoba12 Dec 30, 2016

Choose a reason for hiding this comment

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

Better message

Spyder was unable to load the <i>Spyder 3</i> icon theme. That's why it's going to
fallback to the theme used in Spyder 2.

For that, please close this window and start Spyder again.

@ccordoba12
Copy link
Member

@dalthviz, I added a commit to your branch to fix some Travis errors. Please don't forget to pick it up locally.

Still this is a bit tricky to merge because it depends on a new version of QtAwesome. I'm going to release QtAwesome 0.4 with your FontError fix, so @dalthviz you need to bump here too our package requirements to that version in all places we define them. And for that, please check how @rlaverde did it for Chardet in PR #3640.

@goanpeca
Copy link
Member

goanpeca commented Dec 30, 2016

@ccordoba12 regarding

...you need to bump here too our package requirements to that version in all places we define them.

I suggest we use a requirements.txt file (or more as needed) as a single entry point for modifying dependencies (we can rely on this file for all CI services and scripts). This will make the updating of dependencies less error prone. I implemented this for another project and it eases the pain :0)

@ccordoba12
Copy link
Member

I suggest we use a requirements.txt file

Three reasons i think this is not a good idea:

  1. We need fine grained control for our dependencies. For example, in CircleCI we need to install qtconsole along with the right PyQt version separately from the rest of our deps.
  2. We're still testing that our conda and pip packages pull the right dependencies on Travis and AppVeyor. And for that, we don't need a requirements file, just let the packages to pull the dependencies.
  3. People still wants to know (in README and our docs) what our dependencies are. Again, we can't use a simple requirements file for that.

I know this process is still cumbersome, but that's why CI services are for: to test if we did things right :-)

@goanpeca
Copy link
Member

goanpeca commented Dec 30, 2016

Regarding the comments

1.) That is the exception and not the rule (in reality is just PyQt ad Python)
2.) So?
3.) Sure we can, we put a link to the file and comment out the dependencies we have to handle "manually" on CI, which is basically for now just PyQt

@goanpeca
Copy link
Member

... reasons i think this is not a good idea:

If I had nickel for every time...

…s in package requirements to version 0.4 of qtawesome.
@ccordoba12
Copy link
Member

@dalthviz, please add the QtAwesome version we need now in README.md and docs/installation.rst as 0.4+.

@goanpeca, sometimes you're right and sometimes not. But you're prone to give me the task of finishing what you start, and I really (really) don't have time for that now. So I think we're going to continue with our awkward (but proven ;-) procedure. As I said before, it's not something we do everyday so I'm fine with it.

@ccordoba12 ccordoba12 added this to the v3.1 milestone Dec 31, 2016
@goanpeca
Copy link
Member

image

@ccordoba12 ccordoba12 changed the title PR: Add a message dialog to inform the way to handle untrusted/corrupted font files. PR: Add a way to bypass errors in QtAwesome Dec 31, 2016
@ccordoba12
Copy link
Member

@dalthviz, please change QtAwesome version to 0.4.1. I had to do some changes in 0.4 to better detect this error.

@@ -145,6 +145,7 @@
PY3, qbytearray_to_str, configparser as cp)
from spyder.utils import encoding, programs
from spyder.utils import icon_manager as ima
from qtawesome.iconic_font import FontError
Copy link
Member

Choose a reason for hiding this comment

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

Please move this import after the qtpy ones, i.e. after line 92

@ccordoba12 ccordoba12 changed the title PR: Add a way to bypass errors in QtAwesome PR: Add a way to handle font errors in QtAwesome Jan 3, 2017
@ccordoba12 ccordoba12 closed this Jan 3, 2017
@ccordoba12 ccordoba12 reopened this Jan 3, 2017
@ccordoba12
Copy link
Member

Ok, this is ready now. Thanks a lot @dalthviz for your work on this one. It's very important for 3.1 :-)

@ccordoba12 ccordoba12 merged commit 2ca128a into spyder-ide:3.x Jan 3, 2017
ccordoba12 added a commit that referenced this pull request Jan 3, 2017
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.

3 participants