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

UI: Prefix tooltips with what kind of information they show #3327

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@jostephd
Copy link
Member

commented Jul 7, 2018

Having these prefixes is consistent with all other tooltips in the sidebar.

Also add a tooltip to vision because the label is truncated and there is no other way for the player to get the vision value.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

Looking at this makes me want to add jamming to the vision tooltip, too (if it's non-zero).

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2018

I tried to add that:

diff --git a/src/reports.cpp b/src/reports.cpp
index 2a48fc5e7c4..d8a12dfe72b 100644
--- a/src/reports.cpp
+++ b/src/reports.cpp
@@ -571,6 +571,11 @@ static config unit_vision(const unit* u)
 		str << _("vision: ") << u->vision();
 		tooltip << _("Vision: ") << u->vision();
 	}
+	if (u->jamming() != 0) {
+		if (str.tellp() == 0)
+			str << _("jamming: ") << u->jamming();
+		tooltip << _("Jamming: ") << u->jamming();
+	}
 	return text_report(str.str(), tooltip.str());
 }
 REPORT_GENERATOR(unit_vision, rc)

But in wesnoth -t it had no effect. The scenario has two units with [unit] jamming=5 set, an Outrider and an Orcish Archer, but in :inspect both of them show with jamming=0, and :unit jamming=5 doesn't change that.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

Just for the record, I think you'd also need a newline between the two entries, wouldn't you?

That's interesting about jamming not working though. Does it show in :inspect? Maybe the unit needs to also set max_jamming or something? I dunno. I actually haven't ever gotten around to trying jamming yet (though I intend to eventually as I think it's a cool concept, particularly for someone making a more sci-fi era).

@Vultraz
Copy link
Member

left a comment

Please don't include spaces in the translated strings.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

I'll add the newline and remove the trailing spaces.

@CelticMinstrel As I said, :inspect shows jamming=0. The wiki doesn't document a jamming key in [unit] or [unit_type], only in [effect]. I think the test scenario may be using a syntax that isn't supported. I will investigate.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

I looked into it and it seems that it has only ever been supported in [unit_type], not in [unit].

fbbfadf#diff-5c3015c1ec6963a9da9a75fff346f390R788

I think this is an oversight that should be fixed; however, I guess that would need to be on master only.

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2018

Thanks. I can add that to my todo list after the newline and whitespace.

Before I forget, using :kill on a jamming unit (Mages) doesn't redraw the vision of the jammed unit (Konrad). Here is a test:

diff --git a/data/campaigns/Heir_To_The_Throne/scenarios/01_The_Elves_Besieged.cfg b/data/campaigns/Heir_To_The_Throne/scenarios/01_The_Elves_Besieged.cfg
index 805f25cc679..293de472be0 100644
--- a/data/campaigns/Heir_To_The_Throne/scenarios/01_The_Elves_Besieged.cfg
+++ b/data/campaigns/Heir_To_The_Throne/scenarios/01_The_Elves_Besieged.cfg
@@ -49,6 +49,9 @@
                 carryover_percentage=40
             [/gold_carryover]
         [/objectives]
+        {UNIT 2 Mage 21 21 (vision=1)}
+        {UNIT 2 Mage 22 21 (vision=1)}
+        {UNIT 2 Mage 23 21 (vision=1)}
     [/event]
 
     {HTTT_INTRO}
@@ -68,6 +71,7 @@
     [side]
         type=Fighter
         id=Konrad
+        fog,shroud=yes,yes
         name= _"Konrad"
         unrenamable=yes
         profile=portraits/konrad-elvish.png
diff --git a/data/core/units/humans/Mage.cfg b/data/core/units/humans/Mage.cfg
index e9ea2aff94d..9ce4f57edfc 100644
--- a/data/core/units/humans/Mage.cfg
+++ b/data/core/units/humans/Mage.cfg
@@ -13,6 +13,7 @@
     experience=54
     level=1
     alignment=lawful
+    jamming=1
     advances_to=White Mage,Red Mage
     cost=20
     usage=mixed fighter
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

Just for the record, fixing jamming is out of scope for this PR. Maybe that was obvious but I like to make sure the obvious is explicitly stated anyway, just in case it actually wasn't obvious to someone.

jostephd added some commits Jul 7, 2018

UI: Prefix tooltips with what kind of information they show
Also add a tooltip to vision because the label is truncated and there is
no other way for the player to get the vision value.

@jostephd jostephd force-pushed the jostephd:sidebar-tooltips branch from 85ba12d to a32af45 Jul 9, 2018

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

I've pushed the newline and whitespace fixes. I fixed only the newly-added strings since I didn't want to break translations.

@CelticMinstrel It was clear to me, but thanks for making it explicit. I've submitted #3330 for the [unit] jamming=5 support. Shall I also file a bug about :kill not redrawing?

@sevu sevu added this to the 1.14.6 milestone Sep 9, 2018

@jostephd jostephd self-assigned this Sep 26, 2018

@jostephd jostephd modified the milestones: 1.14.6, 1.14.7 Jan 6, 2019

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

There are merge conflicts, 1.14.6 string freeze starts soon, and this isn't terribly urgent. Postponed to 1.14.7.

jostephd added a commit that referenced this pull request Feb 24, 2019

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

Merged in 97ca22e ce75665.

@jostephd jostephd closed this Feb 24, 2019

jostephd added a commit that referenced this pull request Feb 24, 2019

jostephd added a commit that referenced this pull request Feb 24, 2019

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.