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

(1.8.0.7 release) Do not kill game when failing to find icon on Mac operating systems, … #87

Closed
wants to merge 1 commit into from

Conversation

DanVanAtta
Copy link
Member

…instead print a warning message to the console.

Review on Reviewable

…instead print a warning message to the console.
@DanVanAtta DanVanAtta changed the title Do not kill game when failing to find icon on Mac operating systems, … (1.8.0.7 release) Do not kill game when failing to find icon on Mac operating systems, … Aug 19, 2015
@veqryn
Copy link
Member

veqryn commented Aug 19, 2015

We should have automated test for this, otherwise this bit of code is the only way to know if we built our mac app dmg correctly or not.
I would rather have the error thrown, and we fix it, then not have the error thrown and we never fix it.

@DanVanAtta
Copy link
Member Author

The error would still be on the console, it just would not come to the foreground.

I ran into this same error on a linux system in a dev context, it seemed unnecessary to run into.

As part of our release testing process we should be checking the console window, it can contain other errors that are printed to System.out and not System.err - so by that token we should still see this error. Also as previously discussed, when doing release testing, we'll turn on the future option to always show these errors in foreground.

@DanVanAtta
Copy link
Member Author

The automated test is tough, things depend on our packaging of mac files and then this running on a mac system as well (notice the code is wrapped in a "if mac os" check).

@veqryn
Copy link
Member

veqryn commented Aug 19, 2015

Until we get a mac developer, we can not rely on people to check this stuff for us. I would rather a fatal error pop up, to immediately show that something is wrong.

@DanVanAtta
Copy link
Member Author

We have a number of mac specific items. My impression was that we would find some volunteers with a mac to do testing, we would instruct those volunteers to turn on the "show all errors" flags when we create it, in the meantime they would check the console.

Without doing that, and having so much mac specific stuff, we are letting the end-users do testing for us, testing in prod.

It looks like for the most part our mac specific stuff is largely okay. We do have this block in GameRunner2:

     if (!GameRunner.isMac()) {
              try {
                UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
              } catch (final Exception e) {
              }
            }

By the same rationale of not finding stuff, we should have this throw an illegalState? Looking through, this appears to be the only area of error handling discrepency. The rest of the cases, it appears that if an exception is thrown in the mac specific code, it'll trickle up to the usual error handling that is done for any platform.

@DanVanAtta
Copy link
Member Author

Opened #96 to discuss how to handle mac code that has its own error handling.

@DanVanAtta DanVanAtta closed this Aug 19, 2015
@DanVanAtta DanVanAtta deleted the mac_icon_error_handling branch August 19, 2015 21:26
@veqryn
Copy link
Member

veqryn commented Aug 19, 2015

The icon missing for mac's was not the full error, from my understanding. It just happened to alert us to main problem, and it was the most visible bug to the users since it gave them a big error message.
So look and feel stuff is different than build stuff related to macs. If look and feel can not run, we do not really care.

@DanVanAtta
Copy link
Member Author

Please be explicit about the main problem. It does not seem like there could be any further problems after the IllegalStateException was thrown. The icon is quite similar to look and feel, it's very hard to argue that these are so different.

@DanVanAtta
Copy link
Member Author

Also, if we do not care about look and feel, why do we even have the code. We have no idea if it is working, we never test it, we have no idea if there is benefit, we have no idea if anyone ever actually asked for it rather than it being a nice thing.

If there is benefit then let's log an error when this thing fails, otherwise we'll break it silently. And then we really won't know it was broken since we don't even log a message.

@veqryn
Copy link
Member

veqryn commented Aug 19, 2015

Sorry, but what I mean is that we do not care if the look and feel fails to load, so long as we can revert back. Mac users seem to like their platform's look better than the default look and feel for triplea (raven graphite or whatever), and that is why that if statement is there.
I am fine with logging that there was a problem with loading the look and feel, but I am simply saying why we do not propagate that error up.

@DanVanAtta
Copy link
Member Author

I think in both cases we should log that we could not generate the best UI we could. To me not having the preferred icon is very similar to not having the preferred look and feel. By that token we shouldn't kill the game if we can't get the icon, hence same for the look and feel. But again, I think we should just ClientLogger.logQuietly(..) both problems.

Right now my understanding is that we agree at the very least that we should log something if the look and feel fails to load.

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.

2 participants