Show a unit tooltip when mouse-overing units on main map after 1s. #3477

asvitkine · 2018-06-24T23:28:29Z

This is helpful for new players as well as players trying out new maps that have custom units.

The tooltip delay is set to 1 second, so it shouldn't be too intrusive.

codecov-io · 2018-06-25T00:37:09Z

Codecov Report

Merging #3477 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #3477      +/-   ##
============================================
- Coverage     21.97%   21.97%   -0.01%     
+ Complexity     5919     5918       -1     
============================================
  Files           836      837       +1     
  Lines         71983    72016      +33     
  Branches      11590    11593       +3     
============================================
+ Hits          15820    15822       +2     
- Misses        54075    54103      +28     
- Partials       2088     2091       +3

Impacted Files	Coverage Δ	Complexity Δ
...n/java/games/strategy/triplea/ui/TripleAFrame.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...mes/strategy/triplea/ui/MapUnitTooltipManager.java	`0% <0%> (ø)`	`0 <0> (?)`
...rategy/triplea/attachments/UnitTypeComparator.java	`46.15% <0%> (-7.7%)`	`12% <0%> (-1%)`
...me-core/src/main/java/games/strategy/net/Node.java	`75.51% <0%> (-2.76%)`	`16% <0%> (+1%)`
.../strategy/triplea/odds/calculator/DummyPlayer.java	`35.22% <0%> (-1.14%)`	`9% <0%> (-1%)`
...tegy/engine/framework/lookandfeel/LookAndFeel.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...rc/main/java/games/strategy/net/nio/NioWriter.java	`71.92% <0%> (+3.5%)`	`20% <0%> (ø)`	⬇️

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 b59e309...d25b1cd. Read the comment docs.

Cernelius · 2018-06-26T10:53:08Z

This seems something users may want to be able to customize (Engine Preferences). I think games often give the ability to set the delay tooltip.

Cernelius · 2018-06-26T10:55:18Z

1 second sounds good for me.

RoiEXLab

Code looks mostly good, just a couple of nitpicks from my side.
As I already said in the other PR, we always appreciate Screenshots when there are any UI changes for future reference.

RoiEXLab · 2018-06-27T18:04:06Z

game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java

+    final MouseOverUnitListener mouseOverUnitListener = (units, territory, me) -> {
+      unitsBeingMousedOver = units;
+      tooltipManager.updateTooltip(getUnitInfo());
+    };
    mapPanel.addMouseOverUnitListener(mouseOverUnitListener);


Would you mind inlining this Lambda expression with the next line?

RoiEXLab · 2018-06-27T18:05:27Z

game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java

-                  + ua.toStringShortAndOnlyImportantDifferences(unit.getOwner(), true, false);
-            }
-          }
+          String unitInfo = getUnitInfo();


Now that this calls a method, could you declare this variable final?

RoiEXLab · 2018-06-27T18:08:26Z

game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java

+        if (unitsBeingMousedOver.size() != 1) {
+          unitText = unitsBeingMousedOver.size() + " Units";
+        }
+        unitInfo = "<b>" + unitText + "</b><br>" + unit.getType().getName() + ": "


I know it's copy-pasted, but I'd rather return this directly and return an empty string otherwhise.

RoiEXLab · 2018-06-27T18:09:09Z

game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java

+        String unitText = "Unit:";
+        if (unitsBeingMousedOver.size() != 1) {
+          unitText = unitsBeingMousedOver.size() + " Units";
+        }


The unitText assignment could be done with a ternary operator, so this variable could be declared final

Done, though I find that a bit harder to read.

RoiEXLab · 2018-06-27T18:10:36Z

game-core/src/main/java/games/strategy/triplea/ui/MapUnitTooltipManager.java

+    // Note: Timer not started yet.
+  }
+
+  /** Updates the tooltip. */


Maybe it's just me, but I'd appreciate if this javadoc could be expanded to the "normal" 3 lines

RoiEXLab · 2018-06-27T18:14:53Z

game-core/src/main/java/games/strategy/triplea/ui/MapUnitTooltipManager.java

+  private Popup popup;
+
+  public MapUnitTooltipManager(MapPanel mapPanel) {
+    this.mapPanel = mapPanel;


We probably want to accept a Component/JComponent as argument rather than a specific MapPanel type.
This would make it clear that we only need a "parent component" object.

RoiEXLab · 2018-06-27T18:16:45Z

game-core/src/main/java/games/strategy/triplea/ui/MapUnitTooltipManager.java

+  private String text;
+  private Popup popup;
+
+  public MapUnitTooltipManager(MapPanel mapPanel) {


We have a (perhaps outdated 😉) Style guildeline that makes all variables final which are effectively final.
This includes method and constructor parameters.
The same thing applies to all other methods you created, not going to comment them all individually

asvitkine · 2018-06-28T02:37:57Z

Thanks. Comments addressed.

Here's a screenshot of how this looks.

Cernelius · 2018-06-28T02:47:14Z

I think it would be really good if there would be a menu option to turn this off. This is nice when you are noob with a game, but a lot of users just play the exact same map again and again, and will never want to see this.
I'm thinking the delay should be 1 s if optional or 2 s if you can't turn it off.

asvitkine · 2018-06-28T03:53:11Z

How about letting people try it and see how it feels before prematurely building the option to turn it off? Adding more options increases complexity and it seems better to see if people actually find it annoying after trying it rather than speculatively adding an option that will stay there forever.

RoiEXLab

@Cernelius has a valid point there.
Implementing a simple setting to toggle thid shouldn't be too difficult though.
I'll send you instructions on how to do it later this day, we have a "framework" for this.

RoiEXLab · 2018-06-28T05:19:05Z

(No objections on leaving this on by default though to have people try it out)

asvitkine · 2018-06-28T05:41:33Z

Sure, I'll happily add a setting for it if that's the feedback after people try it out. My point is let's see how the current behaviour feels first. And the setting can be added in a follow-up PR.

Heppisorus · 2018-06-28T18:28:56Z

seems like initially the setting would be good. If after implementation it becomes obstructive to game play I am sure you can extend the delay.

RoiEXLab · 2018-06-28T19:04:55Z

@asvitkine You probably want to have a look at ClientSetting.java, ClientSettingSwingUiBinding.java (and ClientSettingJavaFxUiBinding.java if you want to add the setting to the alpha UI I'm casually working on, you need to set a checkbox in the settings in the testing section to be able to see it).
All you need to do is to add an entry for the desired setting in the desired category.
After that you should be able to query the current setting using ClientSetting.SETTING_NAME.value() or something.

ron-murhammer · 2018-06-28T22:22:31Z

@asvitkine @RoiEXLab I'm alright with moving forward with the change as is for now. We can get it into the pre-release and have a few folks try it out. If it seems intrusive then we can look to add settings for it or increase the delay. Either way long term it would be great to have a setting to turn on/off and set the delay but I'm fine with that in a later PR once we have some feedback. Small incremental progress is fine around minor UI areas.

The popup info we have currently is pretty rough and any steps to improving usability especially for more complex maps is welcome. I can see that revised only players may want to disable it but I also don't think their world will come to an end if they have to see a tooltip once in a while.

Can look to have follow on PR.

Heppisorus · 2018-06-29T17:56:34Z

Started testing this. Feels good. Much easier than having to click on the map then have to press a button.

My initial reaction is that 1 second might be very quick for a ever present automatic function... however since the window itself does not interfere with playability it is less of a concern for me.

Over-all a good addition.

Heppisorus · 2018-06-29T18:03:18Z

Will be very helpful for games with greater complexity. In TWW it would help a new player immensely.

asvitkine · 2018-06-29T21:15:11Z

I also had a chance to play a real game with this. It also felt good to me. (Says the person who implemented it, heh.) My biggest observation is after being used to having these, I was expecting them in other places like combat UI - where we don't have them now. So we could look at wiring those in too. On Jun 29, 2018 2:03 PM, "Hepps" <notifications@github.com> wrote: Will be very helpful for games with greater complexity. In TWW it would help a new player immensely. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3477 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABE8Ap_ypRd1ZsmFRHvd_shQ_sAWT7cks5uBmvngaJpZM4U1WN0> .

asvitkine · 2018-06-30T03:34:13Z

One other issue I noticed is sometimes the tooltip will stay open when another dialog pops up over it.

I can take a look at fixing that - probably by listening to focus events on the game window and hiding tooltips in that case.

Heppisorus · 2018-06-30T15:40:34Z

Like the ideas... this would be most helpful in the purchase screen. But I could also see it being useful on the combat window when having to choose casualties.

Overall I am enjoying it. Again I think my preliminary feeling is growing the more I use this... I would likely move this to a 2 second delay.

asvitkine added 2 commits June 24, 2018 19:26

Show a unit tooltip when mouse-overing units on main map after 1s.

2804cd2

Fix checkStyle errors.

959e48e

RoiEXLab previously requested changes Jun 27, 2018

View reviewed changes

Address review comments.

d25b1cd

RoiEXLab reviewed Jun 28, 2018

View reviewed changes

ron-murhammer merged commit 06c97c3 into triplea-game:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a unit tooltip when mouse-overing units on main map after 1s. #3477

Show a unit tooltip when mouse-overing units on main map after 1s. #3477

asvitkine commented Jun 24, 2018

codecov-io commented Jun 25, 2018 •

edited

Loading

Cernelius commented Jun 26, 2018

Cernelius commented Jun 26, 2018

RoiEXLab left a comment

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

RoiEXLab Jun 27, 2018

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

RoiEXLab Jun 27, 2018

asvitkine Jun 28, 2018

asvitkine commented Jun 28, 2018

Cernelius commented Jun 28, 2018

asvitkine commented Jun 28, 2018

RoiEXLab left a comment

RoiEXLab commented Jun 28, 2018

asvitkine commented Jun 28, 2018

Heppisorus commented Jun 28, 2018

RoiEXLab commented Jun 28, 2018

ron-murhammer commented Jun 28, 2018

Heppisorus commented Jun 29, 2018

Heppisorus commented Jun 29, 2018

asvitkine commented Jun 29, 2018 via email

asvitkine commented Jun 30, 2018

Heppisorus commented Jun 30, 2018

Show a unit tooltip when mouse-overing units on main map after 1s. #3477

Show a unit tooltip when mouse-overing units on main map after 1s. #3477

Conversation

asvitkine commented Jun 24, 2018

codecov-io commented Jun 25, 2018 • edited Loading

Codecov Report

Cernelius commented Jun 26, 2018

Cernelius commented Jun 26, 2018

RoiEXLab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvitkine commented Jun 28, 2018

Cernelius commented Jun 28, 2018

asvitkine commented Jun 28, 2018

RoiEXLab left a comment

Choose a reason for hiding this comment

RoiEXLab commented Jun 28, 2018

asvitkine commented Jun 28, 2018

Heppisorus commented Jun 28, 2018

RoiEXLab commented Jun 28, 2018

ron-murhammer commented Jun 28, 2018

Heppisorus commented Jun 29, 2018

Heppisorus commented Jun 29, 2018

asvitkine commented Jun 29, 2018 via email

asvitkine commented Jun 30, 2018

Heppisorus commented Jun 30, 2018

codecov-io commented Jun 25, 2018 •

edited

Loading