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

Restore co-ordinates to classic theme #4515

Closed
wants to merge 1 commit into from

Conversation

@Wedge009
Copy link
Member

Wedge009 commented Oct 24, 2019

Resolves #4053.

Classic theme lost co-ordinates since at least 1.14.6 - apparently due to work on the battery indicator. Tested to work with both battery and no battery indicator, as well as SP and MP.

No need for master, so I understand, because classic theme is supposed to be dropped in 1.15 (it doesn't look like it has been yet, though).

I don't know how classic theme used to look so it probably doesn't match how it used to be before the battery indicator was added.

Resolves #4053.

Classic theme lost co-ordinates since at least 1.14.6 - apparently due to work on the battery indicator. Tested to work with both battery and no battery indicator, as well as SP and MP.

[ci skip]
@Wedge009 Wedge009 added the UI label Oct 24, 2019
@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 25, 2019

When you merge this, please cherry-pick it to master too. It won't make it any harder to remove the classic theme, but it'll make it easier to revive the classic theme as an addon for 1.16, should someone want to do that.

And if you were basing not your decisions not to forward-port this on bf3ece1, note that it has been forward ported after all, in 1489f5f.

@Wedge009 Wedge009 added the Fwdport label Oct 25, 2019
@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Oct 25, 2019

I'd like it to be tested before I merge because I'm not certain I have it correct.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 25, 2019

As it happens, I just finished testing. The clock doesn't show in the classic theme either before or after this PR.

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Oct 25, 2019

That was the one thing I wasn't sure of. Clock shows for me when there's no battery. But I would call the clock's absence is 'out of scope'... unless it's super-easy to fix it here and now (I'm not sure).

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 25, 2019

I'm not sure it's out of scope. I suspect the clock, the battery, and the terrain description are actually all part of the same problem. Might have something to do with the commented out {COUNTDOWN_THEME}, the timeout argument in there is used by {COUNTDOWN_THEME_STATUS} which isn't commented.

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Oct 25, 2019

I meant in the sense that its disappearance is not caused by my change, nor is it specified in the original report.

However, I did try to have a closer look - part of the issue is that the battery is inserted between the gold and the clock/timer indicators. There is nothing after the clock/timer in the current default theme but the co-ordinates come afterwards in the classic theme.

I spent a few hours on this but to no avail - I can't get the clock/timer to appear when the battery is present (I don't normally deal with WML but the experience did give me a better idea of what the theme code is doing). I do confirm the clock/timer is from the COUNTDOWN_THEME_STATUS in _initial.cfg - removing it means the clock/timer disappears from the top display when the battery is not present.

#define COUNTDOWN_THEME_STATUS FONT_SMALL_SIZE
[report_countdown]
id=report_countdown
font_size={FONT_SMALL_SIZE}
ref=timeout-box-center
rect="=,=-3,+80,+20"
xanchor=fixed
yanchor=fixed
[/report_countdown]
#enddef

It's also used in the default theme.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 26, 2019

Thanks for looking into this. All I know is that if I simply uncomment {COUNTDOWN_THEME} in classic.cfg, I get the clock, battery, and terrain info all shown, just in the wrong place:

2019-10-26-005855_363x35_scrot

So, I guess that if you make the clock show in the right place, the battery and terrain info will show up correctly too.

Do we have reason to believe this is a regression caused by adding battery to the status bar? If show, we should tag the people who added the battery and invite them to help.

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Oct 26, 2019

Oh, I didn't get that, but I think I only tested the non-battery case. I may have a look again in the battery case later.

The issue does indeed appear to have been introduced by the battery work but I haven't done any checking to confirm this. I don't know for certain who worked on the battery display but history for classic.cfg suggests it was @hrubymar10 in August last year. @jyrkive also adjusted the classic theme later on in November.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 26, 2019

For the record, the screenshot in my last comment was produced with does_device_have_battery hardcoded to true:

patch
--- a/src/desktop/battery_info.cpp
+++ b/src/desktop/battery_info.cpp
@@ -31,15 +31,7 @@ namespace battery_info {
 
 bool does_device_have_battery()
 {
-#if defined(_WIN32)
-	return windows_battery_info::does_device_have_battery();
-#elif defined(__APPLE__)
-	return desktop::battery_info::apple::does_device_have_battery();
-#elif defined(HAVE_LIBDBUS)
-	return dbus::does_device_have_battery();
-#else
-	return false;
-#endif
+	return true;
 }
 
 double get_battery_percentage()
@jyrkive

This comment has been minimized.

Copy link
Member

jyrkive commented Oct 26, 2019

Did you see this already?

# If the device has no battery, remove the battery charge indicator
# and move the timer to its position.
[no_battery]
[remove]
id=battery
[/remove]
[remove]
id=timeout-panel
[/remove]
[remove]
id=time-icon
[/remove]
[change]
id=report_countdown
ref=battery-panel
rect="=+5,=+1,+70,+18"
[/change]
[/no_battery]

The logic to apply the [no_battery] element if the device doesn't have a battery or enough space for the battery icon is here:

wesnoth/src/theme.cpp

Lines 761 to 768 in 0ad4c14

// Battery charge indicator is always hidden if there isn't enough horizontal space
// (GitHub issue #3714)
static const int BATTERY_ICON_MIN_WIDTH = 1152;
if(!desktop::battery_info::does_device_have_battery() || screen_dimensions_.w < BATTERY_ICON_MIN_WIDTH) {
if(const config& c = cfg.child("no_battery")) {
modify(c);
}
}

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Oct 26, 2019

Yes, and that bit seems to work but it's the clock and co-ordinates after it that are problems. I did think it a bit strange that the time panel gets removed but the black panels aren't named visually so the player doesn't know it was originally for the battery.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 26, 2019

@jyrkive I did see that block, and that's exactly why I tested the "have battery" case. I was dividing and conquering.

@jyrkive

This comment has been minimized.

Copy link
Member

jyrkive commented Oct 26, 2019

The clock is the report_countdown element. Maybe the coordinates are another element that should also be moved together with it?

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 26, 2019

Looked a bit more. {COUNTDOWN_THEME} refers to a battery-box-topright which doesn't exist in the classic theme. With the following patch I get:

2019-10-26-091311_508x29_scrot

which isn't quite right, the time/location/terrain reports don't have their background, and the location report has some other thing below it, but at least the information in there.

patch
diff --git a/data/themes/_initial.cfg b/data/themes/_initial.cfg
index 3f2b42c6eaf..ab5205e6c91 100644
--- a/data/themes/_initial.cfg
+++ b/data/themes/_initial.cfg
@@ -386,7 +386,7 @@
 #enddef
 
 #define COUNTDOWN_THEME
-    {STATUS_BOX_BORDERLESS +3 =+0 +90 +15 timeout battery-box-topright fixed fixed}
+    {STATUS_BOX_BORDERLESS +3 =+0 +90 +15 timeout battery-panel fixed fixed}
 #enddef
 
 #define COUNTDOWN_THEME_STATUS FONT_SMALL_SIZE
diff --git a/data/themes/classic.cfg b/data/themes/classic.cfg
index dfc13215ce0..f361e31a86e 100644
--- a/data/themes/classic.cfg
+++ b/data/themes/classic.cfg
@@ -177,7 +177,7 @@
             yanchor=fixed
         [/panel]
 
-        #  {COUNTDOWN_THEME}
+        {COUNTDOWN_THEME}
         [panel]
             id=timeout-panel
             image=themes/legacy/status-bg.png

This patch shouldn't be committed, it will break the default theme. I'm just posting it to help pinpoint the problem. The reason it works is that the classic theme doesn't have a battery-box-topright.

@jyrkive As far as I can see, the time/coordinates/terrain are three separate elements and changing the location of the first moves the second and third too. It'll probably be these:

# This panel encloses the location information displays and the
# observer icon. This separate container is used so that we can
# make the terrain name display stretch to fill all available space
# so that the long strings don't get cut off as easily.
[panel]
id=terrain-panel
rect="+0,=,1024,="
xanchor=left
yanchor=fixed
[/panel]
# The size of these rectangles only accommodates hex coordinates
# up to 99. If either is over that maximum the movement cost will
# be pushed off the right end.
[position]
id=status-position
font_size={DEFAULT_FONT_SMALL}
ref=terrain-panel
rect="=+15,=,+95,="
xanchor=fixed
yanchor=fixed
[/position]
[terrain]
id=status-terrain
font_size={DEFAULT_FONT_SMALL}
ref=terrain-panel
rect="=+115,=,=-24,="
xanchor=left
yanchor=fixed
[/terrain]

@jostephd jostephd added this to the 1.14.10 milestone Oct 26, 2019
@jyrkive

This comment has been minimized.

Copy link
Member

jyrkive commented Oct 27, 2019

Unfortunately I'm too busy to try to fix this myself. :(

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Nov 1, 2019

I wonder if screen resolution plays a part because no matter what I try - including what's been discussed above - I cannot get the clock/timer to appear when battery is present.

I wonder if we should leave this till the clock/timer absence is resolved or push through so that at least the co-ordinates/terrain are restored.

@Wedge009

This comment has been minimized.

Copy link
Member Author

Wedge009 commented Nov 7, 2019

@Wedge009 Wedge009 added Stable 1.14 and removed Fwdport labels Nov 7, 2019
Vultraz added a commit that referenced this pull request Jan 19, 2020
[ci skip]

This theme has been broken for awhile. Removing so we don't ship it in 1.14.10. Closes #4515.
@Vultraz Vultraz closed this Jan 19, 2020
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.