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

Error Dialog improvements and Pyfa.py refactoring #1048

Merged
merged 19 commits into from Apr 11, 2017

Conversation

Projects
None yet
3 participants
@Ebag333
Copy link
Contributor

commented Mar 16, 2017

Reworks the error dialog so the layout is a bit cleaner, and we get more information. Hopefully users will copy/paste, so this tells us more about their environment.
https://puu.sh/uLgXZ/3a3abef8d4.png

The big change in here is Pyfa.py. Functionally it's the same, but there's been a lot of low hanging fruit in here that has irritated me for a long time. In no particular order:

  1. Move a bunch of imports to the top (basically everything that can go to the top does).
  2. Wrap the ones we expect might fail (wxPython and sqlAlchemy) in a try/catch so we don't fail at the top. This lets us fail later, when we can show a pretty error dialog and log properly to the log file.
  3. Rework the detection logic for wxPython. For some reason, the function we were using has some hard requirements for when you use it, instead just test for what version we have installed. Also, adds a bit of logging to show what version is being used, useful for devs or if stack traces happen and logs dump.
  4. Instead of exiting when we hit a problem (see 2 or 3 above), log it and then raise an exception. This means that you get the nice error handling and not just a dump to stdout().
  5. Encapsulate (nearly) everything in a try/catch for the error handling. Basically if something fails in pyfa.py, we're going to get the nice error dialog. The one big exception is the main loop, which can't be embedded in there.
  6. Adds a todo at the bottom. We currently don't really do anything when Pyfa ends, even though we have a bunch of sub-threads. We should be cleaning up after ourselves. This is a place holder for a future change.
@codecov-io

This comment has been minimized.

Copy link

commented Mar 16, 2017

Codecov Report

Merging #1048 into development will decrease coverage by 0.03%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##           development    #1048      +/-   ##
===============================================
- Coverage        10.19%   10.16%   -0.04%     
===============================================
  Files             2139     2139              
  Lines            31402    31516     +114     
===============================================
  Hits              3203     3203              
- Misses           28199    28313     +114

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 878d9d7...48db3c8. Read the comment docs.

Ebag333 added some commits Mar 18, 2017

Add unhandled exception handler. Now catches problems and will try an…
…d output to the pyfalog, falling back to outputting to the console.
@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@blitzmann I just figured out how to make the error window pop up for all uncaught exceptions.

This eliminates the try/catch that we had.

I also figured out how to get it to output to both the pyfa log and the console when it stacktraces, so there's no need to avoid redirecting StdErr anymore.

I also tested the frozen framework we have, and I can't find any issues with redirecting, so all that's a lot simpler now. (We still try/catch those in case there's problems redirecting, so even if something prevents redirection it won't break.)

Adds a bunch of debug level information such as version information for wxPython/sqlAlchemy, if we're frozen or not, where we're trying to write the logfile (might help troubleshoot the Gentoo issue), etc.

Basically this greatly simplifies things. It removes almost all the frozen checks (for some reason, wxPython doesn't properly show a version when frozen, it might be because of the way the skeleton was built), and eliminates a lot of the multiple ways of doing things.

Here you can see what it looks like when an unhandled exception is generated (this one I caused by forcing it to trip over wrong Python version installed):
https://puu.sh/uOWZV/d021f4511b.png

Goes to the log, the error display window, and the IDE. And the IDE even has clickable links!

I think this one is ready to merge.

Ebag333 added some commits Mar 18, 2017

Python now properly requires 2.7 and not 2.6 or 2.7. Removed duplicat…
…e output to log, now that we handle exceptions.
Add logging OS version. Deprioritize some messages to debug, so devs …
…can run at info level with less spam (and much less impact on recalc times).
@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2017

Looks good on the surface, need to test builds first as well as functionality with lacking python modules. Just need to remove them and test, should be able to either merge or request changes by eod today.

for some reason, wxPython doesn't properly show a version when frozen, it might be because of the way the skeleton was built

How do you mean? My wx version in Help > About shows the correct version in the built package.

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2017

How do you mean? My wx version in Help > About shows the correct version in the built package.

Different way of getting the version. I changed the way we look for it in pyfa.py because the way we did it had a bunch of requirements that made it more difficult to run the import at the top than it needed to be (had to be run in a certain order, couldn't have wx active, etc).

It's fine, not a big deal.

I should also probably rebuild this PR.

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2017

Different way of getting the version. I changed the way we look for it in pyfa.py because the way we did it had a bunch of requirements that made it more difficult to run the import at the top than it needed to be (had to be run in a certain order, couldn't have wx active, etc).

Are you talking about wxversion.select() stuff? Unfortunately that is something that is required IIRC if you have multiple versions of wx installed. I just ran --wx28 which is supposed to force using 2.8 and it gave me 3.0. This is for compatibility purposes. I get this on output of that check line: wxPython is installed. Version: ['3.0-msw', '2.8-msw-unicode] (forced).

Aside from that, I tested all builds and it seems to work great! A bit of styling issues of the error dialog on OS X, but I'm not concerned about it.

One thing that I want to bring up: you are making exceptions out of the pre-checks, eg:

elif wx is None or wxversion is None:
            exit_message = "\nCannot find wxPython\nYou can download wxPython (2.8+) from http://www.wxpython.org/"
            raise Exception(exit_message)
exit_message = "\nCannot find sqlalchemy.\nYou can download sqlalchemy (0.6+) from http://www.sqlalchemy.org/"
            raise Exception(exit_message)

I still need to check how these work when removing packages, but will this raise an excpetion and traceback / error dialog? I don't think we want that. These particular checks are for users running from source code (they shouldn't happen for our builds since we provide the packages needed). It's supposed to be a simple message stating to download the requirements. I don't think we need to scare them with a traceback (and in the case of the wx exception, error dialog wont even happen since wx isn't found). For me, it would be best to just print() and sys.exit(). Please clarify.

But yeah, otherwise this rocks. Having an dialog pop up whenever an exception happens has been something I've wanted for a long, long time. :D

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2017

Are you talking about wxversion.select() stuff? Unfortunately that is something that is required IIRC if you have multiple versions of wx installed. I just ran --wx28 which is supposed to force using 2.8 and it gave me 3.0. This is for compatibility purposes. I get this on output of that check line: wxPython is installed. Version: ['3.0-msw', '2.8-msw-unicode] (forced).

I'll have to take a look at the original code again. It didn't look like it was forcing 2.8 to me, but I'll double check it again.

Given how old 3.0 is, is there still a technical reason for forcing 2.8?

Aside from that, I tested all builds and it seems to work great! A bit of styling issues of the error dialog on OS X, but I'm not concerned about it.

It seems that the font size is larger on OS X, so it wraps oddly. We can probably alleviate that by making the window wider and leaving a gap on the right side between the text and edge of the window.

One thing that I want to bring up: you are making exceptions out of the pre-checks

We were exiting anyway. By handling it as an exception it'll trigger the popup window (assuming wx is there, if not it'll just output to console).

We're actually bailing out in less scenarios as well. Basically only if wx and/or sqlAlchemy are actually missing. There's a bunch of catches now that warn you're on an unsupported version, but tries to run anyway.

I think that raising an exception is appropriate here. If a user is running from code, it makes it very obvious to them (especially if they have wx and get the error window). It also handles it a bit more cleanly, we have a single exit point (unhandled exception handler) where we can make sure that everything is cleaned up properly (threads ended, etc), if we just use sys.exit() then we have to make sure each of those spots is exiting cleanly.

We had 7 sys.exit() statements previously. Now we have 2. :)

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2017

I'll have to take a look at the original code again. It didn't look like it was forcing 2.8 to me, but I'll double check it again.

Yeah. That got missed. Oops.

Given how old 3.0 is, is there still a technical reason for forcing 2.8?

Still wondering about this though.

blitzmann added some commits Mar 26, 2017

Bringing branch up to date with dev
Merge branch 'development' into ErrorDialog_and_miscfixes

Conflicts:
	eos/saveddata/fit.py
	pyfa.py
@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2017

Given how old 3.0 is, is there still a technical reason for forcing 2.8?

Compatibility mostly. I introduced the force flag so that I could test 2.8 as well, which I still do on occasion for OS X deprecated, or so the user can choose to use 2.8 if 3.0 was giving them issues (which isn't really a reason anymore, most wx3 bugs have been ironed out).

If it's not a bitch to put back in, please do so, even if the import isn't at the very top of the file. I just honestly don't know how prevalent having to force a version is or not and I"d rather no break someones install. If there's a lot of difficulties with it, then just let me know - may have to just deprecate that option.

If a user is running from code, it makes it very obvious to them (especially if they have wx and get the error window).

It makes it obvious, but what I was concerned about is that it looks like an application error complete with traceback when its really just letting them know that they have missing requirements (so, informational rather than an unhandled exception that we should be notified about). It works fine as is, but it's a user-perception issue.

But I think I've come up with a good solution for that. Simple wxMessageBox:

image

Brought the branch up to date with dev along with that addition. I think it's a good solution to just inform the user that, yo, you need to install som stuff. Tested a bit in Linux. Can you see anything wrong with it?

Let me know about the select() thing as well

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

But I think I've come up with a good solution for that. Simple wxMessageBox:

So instead of another custom error box, how about this?
https://puu.sh/v0JED/55a657d99f.png

In this instance, I forced Python to have an invalid version number.

Yes, it's the same box, but this way if they post about it, we'll know the environment (assuming they screenshot or copy/paste). Also, if a user is getting the error box often enough that they confuse a stacktrace and this, we need to fire that user. 😁

I also added some more logic to trap errors if bits are missing, so we don't stacktrace when trying to show our stacktrace.

I'm also going to go ahead and rejigger this PR entirely on a new branch (and new PR). This one seems to be a bit messed up, not sure if it's the force push that hit it or something else. Cleaner commit history, and should take care of the two conflicts.

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

So instead of another custom error box, how about this?

It's not a custom error box, it's a built-in wxMessageBox. And I think for simple... well, messages, this should suffice. The one you posted still says "encountered unexpected error blah blah blah". If they get a message box setting "You're python version is not correct" and they come to us, as a Linux user, and say "hey pyfa's broke lol" even though they get an explicit message stating the error, then so be it - users will be users. The current implementation of simple text to the console has worked for years, lets just continue to keep it simple. This is that way I prefer it done. We could expand it though to give the user a little more info (for example, if it's a python version mismatch, give them the version required and tell them the version that they are trying to use). That would suffice. :)

I'm also going to go ahead and rejigger this PR entirely on a new branch (and new PR). This one seems to be a bit messed up, not sure if it's the force push that hit it or something else. Cleaner commit history, and should take care of the two conflicts.

This branch look absolutely fine. It's very clean, and contains only commits pertaining to this feature, along with merges from dev that I did to bring it up to date. You might not be able to rebase it due to that (dunno, never tried), but since this is now a public branch it's probably best not to rebase anyway due to it rewriting histroy. You just have to solve the merge conflicts that have been introduced with recent commits. If you don't want to do that, continue comitting to the branch and I can deal with the merge conflicts when I merge it into dev. :)

Thanks

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

It's not a custom error box, it's a built-in wxMessageBox.

That's a fair point.

The one you posted still says "encountered unexpected error blah blah blah".
The current implementation of simple text to the console has worked for years, lets just continue to keep it simple. This is that way I prefer it done. We could expand it though to give the user a little more info (for example, if it's a python version mismatch, give them the version required and tell them the version that they are trying to use). That would suffice. :)

Did you look at the screenshot? ;)

It no longer says that. And it has in giant letters across the top "Missing Prerequisite".

As for keeping it simple, this uses a single method of generating the error window rather than two different ones. I'd say that's even simpler.

It would also help us when figuring out what's different between someone's environment and yours/mine in a single go rather than having to query for each bit of info separately.

Speaking of which, we should probably add the logbook version since that's pretty critical.

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

This branch look absolutely fine. It's very clean, and contains only commits pertaining to this feature, along with merges from dev that I did to bring it up to date. You might not be able to rebase it due to that (dunno, never tried), but since this is now a public branch it's probably best not to rebase anyway due to it rewriting histroy. You just have to solve the merge conflicts that have been introduced with recent commits. If you don't want to do that, continue comitting to the branch and I can deal with the merge conflicts when I merge it into dev. :)

I wasn't trying to rebase, you shouldn't rebase public branches as you said. If I was the only one working on it that'd be one thing, but since you have commits I wouldn't do that to you. :)

Anyway, when doing a pull (because my local branch is behind, and missing your commit) it wants to merge changes to 19 files:
https://puu.sh/v0OGy/96d2af7539.png

Considering only 5 files changed, something is a little off here.

Anyway, I was just trying to make things easier for you to merge it into dev. If you want to do more work, who am I to say no? :)

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2017

Did you look at the screenshot? ;)

I did. Did you read the paragraph underneath it? It still makes it sound like an error.

I would prefer the error dialog to be used for just that: handling uncaught exceptions. It does that really well. These aren't errors, they are notifications to the user that they need to get their prereqs. Simple text on console sufficed for years, a message box is an upgrade for them. As stated before, it's a perception thing as well as the simple fact that these aren't errors and thus don't need this huge "oh shit things broke" looking window. I would like the MessageBoxes left in place if you don't mind. :)

Speaking of which, we should probably add the logbook version since that's pretty critical.

This makes me think... could we print out a list of all python modules and their versions? Kind of like how pyCharm lists the packages and versions in their package manager. Doesn't need to go in the error dialog, but rather at the start of the log file (or maybe at the end of the text in the error dialog)?

Anyway, when doing a pull (because my local branch is behind, and missing your commit) it wants to merge changes to 19 files:

Just a guess since I don't know what your local repo looks like, but I would assume it's simply because I have merged dev into this branch a couple of times. So when trying to merge it back to your local branch it's letting you know that those files have changed (because they have with respect to your local branch).

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

I did. Did you read the paragraph underneath it? It still makes it sound like an error.

I edited it so it didn't use the word error. I mean, we're forcing the application to bail out, so it really is something that's stopping execution.

I would prefer the error dialog to be used for just that: handling uncaught exceptions. It does that really well.

This tends to be a trend, we have a ton of things in Pyfa that are simply something else handled slightly differently, or two things that could be handled the same way but are handled entirely differently for <reasons>. Stopping execution for a raised exception? Do this. Stopping execution for a raised custom exception? Do the exact same thing, but in a completely different manner.

I like to reuse things, especially when it takes all of about 30 seconds to reuse them. Less branches in the code, less duplication of effort, less complexity. Plus you built a really super nice window, I want to show that sucker off. :)

thus don't need this huge "oh shit things broke" looking window.

The only person who knows it's the "oh shit things broke" window is you and me, and only because it got named "ErrorWindow" and not "ExceptionHandlerWindow."

I would like the MessageBoxes left in place if you don't mind. :)

I'll put it back in, but consider a complaint filed about needless complexity and duplication of effort. 😜

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

This makes me think... could we print out a list of all python modules and their versions? Kind of like how pyCharm lists the packages and versions in their package manager. Doesn't need to go in the error dialog, but rather at the start of the log file (or maybe at the end of the text in the error dialog)?

You mean like this?

[2017-03-29 01:39:34.981000] INFO: __main__: Imported Python Modules:
[2017-03-29 01:39:39.116000] INFO: __main__: eos: 
[2017-03-29 01:39:39.116000] INFO: __main__: sqlalchemy: 1.1.5
[2017-03-29 01:39:39.117000] INFO: __main__: service: 
[2017-03-29 01:39:39.117000] INFO: __main__: platform: 1.0.7
[2017-03-29 01:39:39.117000] INFO: __main__: os: 
[2017-03-29 01:39:39.117000] INFO: __main__: inspect: 
[2017-03-29 01:39:39.118000] INFO: __main__: traceback: 
[2017-03-29 01:39:39.118000] INFO: __main__: sys: 
[2017-03-29 01:39:39.118000] INFO: __main__: re: 2.2.1
[2017-03-29 01:39:39.119000] INFO: __main__: wxversion: 
[2017-03-29 01:39:39.119000] INFO: __main__: config: 
[2017-03-29 01:39:39.119000] INFO: __main__: wx: 3.0.2.0

Not sure we want to put it into the error dialog text. That could make it very long.

Just a guess since I don't know what your local repo looks like, but I would assume it's simply because I have merged dev into this branch a couple of times. So when trying to merge it back to your local branch it's letting you know that those files have changed (because they have with respect to your local branch).

I'm pretty sure it's something messed up in my local branch.

Normally when I pull changes, if the file hasn't changed then pyCharm just brings it down (fast forward, basically). In this case, it's trying to merge changes into files that haven't changed (according to my local branch), and also which match the server.

vOv

I suspect we'll see lots of issues like this until all my local branches have been pulled after those force pushes.

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2017

Stopping execution for a raised exception? Do this. Stopping execution for a raised custom exception? Do the exact same thing, but in a completely different manner.

Yes, please revert back to the MessageBox. It looks cleaner and a message is all the user needs to see to notify them that they are missing dependencies. We also have to consider that we would essentially be maintaining two different cases of the error window - one for pre check exceptions and one for other unhandled exceptions. Having a message box is set and forget. Sure, we stop execution, but not because the program broke, but because the program just plain can't run. I would the like error window to display when an unexpected exception happens. These messages are expected and handled. This is a very nuanced difference, and I can definitely see your point. But to me there is a distinct difference between knowingly throwing an exception due to something and catching shit that breaks to display info.

Complaint noted and filed :P

While in there, if you wouldn't mind adding in a check for requests as per #1089, that would be swell, unless you want to go through the trouble of doing it like we do with the os x deprecated stuff (I would prefer not to and I want to clean up those conditionals eventually. But it doesn't really matter. If you go this route, please open a new PR for that specifically)

:)

@blitzmann
Copy link
Collaborator

left a comment

Revert the removal of the message box. Decide whether you also want to add requests checking in here or do a conditional (see #1089)

kthx

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2017

Revert the removal of the message box. Decide whether you also want to add requests checking in here or do a conditional (see #1089)

I'm working on it. :)

I have an idea for doing all requirements using our handy files.

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2017

I have an idea for doing all requirements using our handy files.

Meaning... ? Taking requirements and automatically generating a message box? As I said in the issue, that is over-engineering the solution. You would probably have to develop out a mapping of package to message, along with versions we support and what not. It just seems more straightforward to just do the checks in pyfa.py and not over-complicate things.

If that is what you are planning and want to try it, be my guest, but please separate that into another PR so that we can discuss the merits of that by itself. Don't want scope creep, and I would like to get this one included into dev asap. :)

thanks!

Clean up requirements file. Add detection and warning for possibly mi…
…ssing requirement. Change back to using the modal.
@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2017

Taking requirements and automatically generating a message box? As I said in the issue, that is over-engineering the solution. You would probably have to develop out a mapping of package to message, along with versions we support and what not. It just seems more straightforward to just do the checks in pyfa.py and not over-complicate things.

Added a first pass implementation. It doesn't version check, just simple checks if the module is available to import. No modal either, if someone's running from the source files they'll have the console available, we'll just warn them that they might be missing requirements.

Don't think it's overly complex either as it's all of 13 lines. :)

Flagging this one as done, we can iterate on the requirements checks.

@Ebag333 Ebag333 added the done label Apr 2, 2017

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2017

Flagging this one as done, we can iterate on the requirements checks.

@blitzmann are we ready to merge this?

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2017

@blitzmann are we ready to merge this?

No, with the amount of changes I would need to fire up linux and os x and remove some packages to test the requirements bit. Give me a bit of time

from imp import find_module

this concerns me - we tried to do this with graph frame and it broke since the skel doesn't have imp in it.

otherwise it seems find on first glance

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2017

Testing this out today, will merge or update as needed. :D

One thing I did notice is that linux users don't get the requirements.txt file, so iterating through that will only work for git users at the moment. Which I guess isn't so bad (and I'm sure we can include that file for Linux users if needed)

@Ebag333

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2017

One thing I did notice is that linux users don't get the requirements.txt file, so iterating through that will only work for git users at the moment. Which I guess isn't so bad (and I'm sure we can include that file for Linux users if needed)

Well, easy enough to include it. And anyone who just grabs the source code should get it.

For the "builds" of Pyfa we don't actually control, not much we can do about that.

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2017

Well, easy enough to include it. And anyone who just grabs the source code should get it.

Indeed.

So far so good, works as expected. The only thing: we can spawn multiple dialogs. I don't think this is too big of an issue (it certainly gets the point across that something is broken), but we may want to look into it.

Did this by raising an exception in an effect that can be projected, and then trying to open a fit that had a projected fit using that module (in this case, tracking disruptor):

image

Again, don't think this needs to be solved right now, but simply keep it in mind for a future improvement.

Checking OS X now, if that works, will be merging this in. Awesome stuff

@blitzmann

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2017

@Ebag333 tested on all three platforms, seems to work well other than the multiple error dialogs. If you're interested in fixing that now, go for it and then I can merge that in as well, otherwise please squash and merge this into dev. :)

@blitzmann blitzmann merged commit 47ea12f into pyfa-org:development Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ebag333 Ebag333 deleted the Ebag333:ErrorDialog_and_miscfixes branch May 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.