search

dmusican / Elegit

A GUI client for people who want to learn Git.
MIT License
30 stars 7 forks source link

Added git revert help button to link to webpage #643

Closed terwilligers closed 5 years ago

dmusican commented 5 years ago

Thanks for the PR, Sam!

I see a few problems that need fixing.

  1. The really easy one: instead of using java.awt.Desktop, can you import java.awt.Desktop and just use Desktop? This would be consistent with the style everywhere else.

  2. The easy one: you've put a try/catch around your code and left the catch empty. This is dangerous, since if an exception happens, we'll never know about it. This is a common idiom in Java because if it is a checked exception, you're stuck figuring out how to propagate it. How to fix this is a very controversial topic. I've settled for a perspective from Bruce Eckel (site seems to be done at the moment) where he proposes wrapping the exception in a RuntimeException, and then you can at least re-throw it. Anyway, search through the Elegit code and find examples of use of the ExceptionAdapter class. You can fix this example by tossing this line of code:

throw new ExceptionAdapter(e);

... in your empty catch clause.

  1. With the buttons now added to the dialog box, at least on my Linux machine, the full text in that dialog is no longer visible; it trails off with "...". Do you see the same? If so, can you make the dialog bigger so it has room the for text?

  2. On my Linux machine, the clicking on the URL causes Elegit to lock up. This is apparently a known problem. I'm amazingly amused by this because I had said to you "it's just a link, why create an automated test?" when the automated test would have caught it. Well, maybe. This has to actually integrate with the rest of the OS, so who knows what would happen? I'll admit I like the second fix linked above; it just means that on my system, the link won't go. So we need to fix it. What would make the most sentence? Do you want to try the above fix? Try it on a Linux VM? Or would you rather me make that fix after you update the above, since I can easily test on my machine?

terwilligers commented 5 years ago

I fixed and commited the the simple Desktop and Exception Issues, but I think problems 2 and 3 would be fixed easier on your Linux machine since you can test them there. For number 2, the window size problem doesn't occur on my Mac.

dmusican commented 5 years ago

@terwilligers I'm pretty sure I fixed the remaining problems. At least, they now work for me. Can you verify that I didn't break anything for you, i.e. that the dialog box and help buttons still work as you intended?

terwilligers commented 5 years ago

I ran it on my machine and everything still works as intended!

dmusican commented 5 years ago

Tests all pass locally for me; Travis is coughing about Java version. That's next up for fixing, but in the meantime, this pull request is great.