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

Add: Unit Scroller UI Feature #4960

Merged
merged 9 commits into from Aug 12, 2019

Conversation

@DanVanAtta
Copy link
Member

commented Aug 12, 2019

Overview

Unit Scroller is a feature largely intended to make it easier for users to ensure they have moved all units. The feature adds UI components to help map to hot keys that will select next and previous units. The scroller features a 'center on current' button and a thumbnail avatar to display the units in the current territory and the name of the current territory.

Notable updates and changes:

  • space bar skips current units. They will not be highlighted after pressing 'f'
  • 'm' is added as a hotkey for previous units (complements 'n' which selects next units)
  • 'c' is added as a hotkey to center on current units
  • Unit scroller is invoked whenever the game step changes or a unit is moved. This is so we can redraw the scroller with the current units.
  • GameData event listener design updated. There a few different kind of listeners, some specific and a very generic "anything changed" listener that the new model is intended to help replace. The previous variant is now marked as deprecated. The new variant uses an enum to identify specific events and calls listeners that are stored in a map keyed by the data event.
  • GameStepListener refactored to use new game data event listener framework. Helps consolidate game data listeners to 'GameData'
  • Gradle is updated to download images, 'png' is added to gitignore and it is intended that any existing and future images would be downloaded and not checked in.
  • Refactor of highlight units, it does not need to be a map as the territory key is never used, can instead be a collection of lists.
  • Map centering code moved to the new unit scroller and updated to be able to scroll to previous units.

Functional Changes

  • Adds unit scroller UI feature

Manual Testing Performed

  • Extensively tested single player game
  • Tested network game, hosted locally and connected locally

This new feature highlighted and revealed these existing bugs:

  • "N" Key Does not work after moving units (Multiplayer only) #4959
  • "Moved Units 'Hide' Unmoved Units when highlighting moveable units" #4957

A bug in this feature that I found and was not able to work out is rendering on multiplayer games. The initial draw renders too much space for the icons. In this situation the highlight current and sleep buttons are drawn too far above and below the other buttons. But, after a short delay (about half a second) swing recomputes the component sizing and the scroller renders correctly. It's a brief and slightly noticeable problem. Single player perhaps has the same issue but the re-rendering is quite fast and it's not even clear if this is also a problem on single player. Overall this bug is quite minor.

Screen Shots

Before

Screenshot from 2019-08-11 19-39-57

After

Screenshot from 2019-08-11 19-39-01

Add: Unit Scroller UI Feature
Unit Scroller is a feature largely intended to make it easier for users to ensure they have moved all units. The feature adds UI components to help map to hot keys that will select next and previous units. The scroller features a 'center on current' button and a thumbnail avatar to display the units in the current territory and the name of the current territory.

Notable updates and changes:
- space bar skips current units. They will not be highlighted after pressing 'f'
- 'm' is added as a hotkey for previous units (complements 'n' which selects next units)
- 'c' is added as a hotkey to center on current units
- Unit scroller is invoked whenever the game step changes or a unit is moved. This is so we can redraw the scroller with the current units.
- GameData event listener design updated. There a few different kind of listeners, some specific and a very generic "anything changed" listener that the new model is intended to help replace. The previous variant is now marked as deprecated. The new variant uses an enum to identify specific events and calls listeners that are stored in a map keyed by the data event.
- GameStepListener refactored to use new game data event listener framework. Helps consolidate game data listeners to 'GameData'
- Gradle is updated to download images, 'png' is added to gitignore and it is intended that any existing and future images would be downloaded and not checked in.
- Refactor of highlight units, it does not need to be a map as the territory key is never used, can instead be a collection of lists.
- Map centering code moved to the new unit scroller and updated to be able to scroll to previous units.
@codecov-io

This comment has been minimized.

Copy link

commented Aug 12, 2019

Codecov Report

Merging #4960 into master will decrease coverage by 0.02%.
The diff coverage is 8.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4960      +/-   ##
============================================
- Coverage     24.11%   24.09%   -0.03%     
- Complexity     6723     6742      +19     
============================================
  Files           978      986       +8     
  Lines         77416    77629     +213     
  Branches      11631    11635       +4     
============================================
+ Hits          18669    18701      +32     
- Misses        56603    56783     +180     
- Partials       2144     2145       +1
Impacted Files Coverage Δ Complexity Δ
.../games/strategy/engine/framework/AbstractGame.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...in/java/games/strategy/engine/framework/IGame.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...trategy/engine/data/changefactory/RemoveUnits.java 73.33% <ø> (ø) 4 <0> (ø) ⬇️
...s/strategy/engine/data/changefactory/AddUnits.java 93.33% <ø> (ø) 4 <0> (ø) ⬇️
...va/games/strategy/triplea/ui/menubar/HelpMenu.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...games/strategy/triplea/image/UnitImageFactory.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/games/strategy/engine/framework/ClientGame.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...y/triplea/ui/unit/scroller/AvatarPanelFactory.java 0% <0%> (ø) 0 <0> (?)
...main/java/games/strategy/triplea/ui/MovePanel.java 0.75% <0%> (+0.01%) 4 <0> (ø) ⬇️
...ain/java/games/strategy/triplea/ui/PlacePanel.java 0% <0%> (ø) 0 <0> (ø) ⬇️
... and 31 more

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 2cf7076...e2d32d6. Read the comment docs.

.gitignore Show resolved Hide resolved
} catch (final IOException e) {
throw new RuntimeException("Unable to load image at path: " + path.getAbsolutePath(), e);
}
}

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Aug 12, 2019

Member

Hmm I wonder if this is really necessary. Typically images are loading using a subclass of ImageFactory which uses ResourceLoader to get the URL.

So if we want ResourceLoader to be completely independent of the graphics framework, then this method should be removed and ImageFactory should be used instead.
If instead the actual loading of ImageFactory should be completely encapsulated in ResourceLoader then ImageFactory should IMO get modified to actually make use of this method to have a single place where all the logic lives.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 12, 2019

Author Member

Interesting consideration. My thinking was ResourceLoader controls access, at least it has the reference to the 'assets' folder and ImageFactory was not appropriate.

  • subclassing is bad way to re-use code. At the risk of sounding religious, it's evil and needs to be used very judiciously.
  • ImageFactory brings in a lot of unnecessary code and constraints. Caching the image is not needed and perhaps not desired. ImageFactory is documented as wanting to be called from the EDT, that is not desired for the case of unit scroller where the images can be fetched off the EDT thread.

Encapsulating access of resourceloader through ImageFactory to simply encapsulate the access seems like unnecessary indirection IMO. To boot though, resource loader is used directly by other classes.

In essence all that is really needed is a non-throwing version of ImageIO and the reference to the assets folder. Ideally this code would not be stateful, but we have the reference to assets folder that is being borrowed.

Looking for some sort of better option, I think simply moving the image loading code to its own utility class is not a bad choice: 8f5cb32

ImageFactory has another problem where it wants URL paths. That is a small problem as we can easily create File paths, creating a URL path to a file system entity is less ideal.

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Aug 12, 2019

Member

Well sub-classing has its own use cases where it is superior in comparison to other ways to layout code.
That being said I don't think the whole image factory code uses object orientation very well and would benefit heavily from composition because that'd allow better unit testing because the filesystem wouldn't be required for all the "subclasses" that currently exist.
Also I think the factory class is currently misused in a lot of cases:
Because ImageIO#read is a blocking operation (at least to my knowledge), all of the UI code that tries to add a button with an ImageIcon will load the Images on the UI Thread.
I believe IO operations like this in general to be the main reason why starting the game causes the UI to freeze for a couple of seconds. I tried fixing this by using futures, but this really didn't work too well.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 12, 2019

Author Member

FWIW, the current branch does not load the unit scroller images until the move phase. We could potentially do a background thread pre-fetch. I think that probably would be a good thing to do. It's quite a lot to add it in this update though.

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Aug 12, 2019

Member

Thinking about this I should probably just created a subclass of Button that takes a URL or InputStream and loads the actual icon in a different Thread to later add it back to itself.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 12, 2019

Author Member

Large images could have a noticeable re-rendering. I was more thinking a background thread launched after the application is launched. There is a lot of 'dead-time' in the selection menus when loading a game where the game could be pre-fetching images that will be used for sure.

@@ -100,6 +102,11 @@ private void clearImageCache() {
icons.clear();
}

public Image getImage(final UnitCategory unit) {
return getImage(unit.getType(), unit.getOwner(), (unit.getDamaged() > 0), unit.getDisabled())
.orElseThrow(() -> new RuntimeException("No unit image for: " + unit));

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Aug 12, 2019

Member

I'm not sure if it's good practice to throw raw RuntimeExceptions.
I'd rather throw IllegalState or IllegalArgument exception instead, but maybe that's just preference.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 12, 2019

Author Member

It's not an anti-pattern necessarily to throw RuntimeException, it depends on context. If it were, the language developers perhaps would have made the class abstract.

I tried really hard to find a great reference @ssoloff pointed me to, it was a guide by the google java people on which exceptions to throw when. RuntimeException was okay to throw when other specific exceptions were not appropriate and when to indicate a "developers messed up" type of situation.

In this case, IllegalState is not appropriate as the data state is fine, the file system has a file in an incorrect location or is completely missing. It's not quite the same as say "stream is already closed", "file has already been read". "IllegalArgumentException" is not appropriate because the argument is fine, the file system state is incorrect (IllegalState is perhaps arguably okay, but not necessarily an improvement).

IMO RuntimeExceptions should be subclassed if there is any expectation for the error to be handled, or if there is a good chance the error will be shown to a user. It is also useful if there is a lot of debugging to be attached. For example we could create a new MissingImageException(File file) that would be somewhat useful. That is a good thing to do IMO for map images, where map makers do have problems of placing image files in the correct place. On the other hand for errors where the devs messed up, and it's a one-off error that we'll always see simply by executing the code, the new exception I think will mostly be code clutter and not add that much value. The value being added is a more descriptive name to the error when the error message in this case makes it clear enough.

I was not able to find any blog posts that clearly stated that runtime exception in this type of situation is an anti-pattern. I do wish I could find that original reference from google on which exception to throw when, it was a great guide on this subject.

I found these two perhaps relevant posts/blogs discussing this topic:

  1. https://dzone.com/articles/implementing-custom-exceptions-in-java See: "1. Always Provide a Benefit"
  2. https://medium.com/@rafacdelnero/11-mistakes-java-developers-make-when-using-exceptions-af481a153397 See: "2 — Creating lots of specific Exceptions"

Don’t create Exceptions for everything. Your application will be full of classes, useless repetition and you will create unnecessary work. Instead, create Exceptions for really important business requirements.

This comment has been minimized.

Copy link
@ssoloff

ssoloff Aug 12, 2019

Member

I tried really hard to find a great reference @ssoloff pointed me to, it was a guide by the google java people on which exceptions to throw when.

I think the reference you're looking for is here.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 12, 2019

Author Member

Indeed, thanks @ssoloff !

Do you have any thoughts on the above conversation? Do you think my line of thinking holds water, or do you think it is always better to sub-class Runtime exception? Curious for the 3rd opinion here.

This comment has been minimized.

Copy link
@ssoloff

ssoloff Aug 12, 2019

Member

It's not an anti-pattern necessarily to throw RuntimeException, it depends on context. If it were, the language developers perhaps would have made the class abstract.

Bloch appears to agree that that should have been the case, but wasn't done to maintain backwards compatibility. Per Item 72: Favor the use of standard exceptions in Effective Java, Third Edition (emphasis Bloch's):

Do not reuse Exception, RuntimeException, Throwable, or Error directly. Treat these classes as if they were abstract.

Personally, I typically try not to throw RuntimeException directly. I agree that creating an excessive number of exception subclasses is probably not the best idea, but having a few generic application-specific subclasses that could be used in various scenarios to keep in the spirit of Item 72 might not be a bad idea.

However, in the present case, it seems you want to simply provide a custom exception message when the Optional is empty; otherwise, you would have just called get(). If so, why not reuse the same exception get() would throw if the Optional were empty? Namely, NoSuchElementException. You don't plan on handling it, so it doesn't really matter what it is as long as it's not RuntimeException.

game-headed/build.gradle Outdated Show resolved Hide resolved
game-headed/build.gradle Outdated Show resolved Hide resolved
@RoiEXLab

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I really like the GameData event change. I mean obviously not all occurrences have been replaced, but it's definitely a step in the right direction.

DanVanAtta added some commits Aug 12, 2019

@DanVanAtta

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@RoiEXLab I think I have updated/responded to all feedback, please let me know if I missed anything or should do more. Very much appreciate you reviewing this change and thank you.

@RoiEXLab
Copy link
Member

left a comment

Yup that should be it. A follow-up PR with all the Collections.singleton(element) would be nice. And also one that does the modulo instead of checking bounds, ideally even Math#floorMod (the real modulo operation) that guarantees that the index will always be in bounds.

@DanVanAtta

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

And also one that does the modulo instead of checking bounds, ideally even Math#floorMod (the real modulo operation) that guarantees that the index will always be in bounds.

IMO extracting the territory search to something functional and writing a unit test would be the way to go there. It's more effort than I will volunteer for in favor of other TripleA work. I'd love to see that done nonetheless, but realistically it's perhaps not the best ROI for at least myself right now.

@RoiEXLab

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Weird LGTM seems to have some issues communicating with the service.
On the website it seems to run normally, but it reports falsely to GitHub

@RoiEXLab RoiEXLab merged commit 4a71534 into triplea-game:master Aug 12, 2019

1 of 2 checks passed

LGTM analysis: Java Analysis failed (could not build the base commit (15b9d4e))
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DanVanAtta DanVanAtta deleted the DanVanAtta:scroller branch Aug 12, 2019

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