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

unit preview pane backports #4394

Open
jostephd opened this issue Sep 28, 2019 · 4 comments

Comments

@jostephd
Copy link
Member

commented Sep 28, 2019

Following these two backports:

0db0b8d
26a30fc

We also need to:

  • copy blades.png to blade.png - as done in master as part of 08c4695 - so that blade attacks show the red icon in unit_preview_pane, like all standard attack types do
  • maybe remove blades.png. Whether we can do this depends on whether blades.png is an API that UMC has been allowed to use. If it's an API, we can't remove it in a patch release. If it isn't, we can.
  • document on the wiki whether images/ is a public API or not. There was some disagreement about this on discord
  • consider backporting 08c4695
  • check whether any other related commits also need to be backported. For example, master also uses the damage types in the unit type help pages, if we backported those two commits we might as well backport that one too.
  • document in the release notes that UMC that uses custom damage types or ranges should add images. The code fails gracefully when there are no images, so it's just a UX question.
@jostephd jostephd added this to the 1.14.10 milestone Sep 28, 2019
@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

The "show damage type and range icons" issues have been collected here: https://github.com/wesnoth/wesnoth/projects/4

Here are the commit messages of some relevant commits. I believe the first ten are from master. I am not sure about the last five: I think they too are from master, but it's possible some of them are commits that were pushed to one of the PRs on https://github.com/wesnoth/wesnoth/projects/4 and were subsequently history rewritten before merge.

I don't have the hashes immediately available.

  • Help: Reorder columns in unit help page

  • Help: Put the icons in the same column as the text that describes them

  • Help: Support optional image in push_tab_pair

  • Help: Use a typedef instead of hardcoding the concrete type

  • Help: Show damage type and range icons in unit help pages

  • unit_preview_pane: Add tooltips to the damage type and range icons

  • unit_preview_pane: Move the icons to before the damage and strikes, as in the sidebar.

  • Break long lines.

  • unit_preview_pane: Add attack icons

  • Sidebar: Add some vertical padding after attacks that have specials.

  • Sidebar: Fix 'damage versus' tooltip not visible

  • Sidebar: If the range icon or damage type icon isn't found, fall back to text.

  • Sidebar: Make the attack icons the same size as the range and damage type icons

  • Sidebar: Show attack, attack range, and damage type icons in the unit_weapons report.

  • Just drop the attack icon for now

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

I'm guessing it would be easier to revert what was already added to 1.14 rather than continue backporting all of this?

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

I'm guessing it would be easier to revert what was already added to 1.14 rather than continue backporting all of this?

Not quite.

What happened here is that 5aaf179 was committed to master and then backported. The backport conflicted so @Vultraz backported two commits that were necessary to fix the conflicts. However, those two commits were part of a larger feature, so either all of the feature should be backported (the blade.png thing because otherwise the UI looks broken, and the rest because there'll be no reason not to backport it once we've backported the first part), or those two commits should be reverted.

I agree with you that it would be easier/better to revert those two commits than to backport the rest of the feature.

Nonetheless, is no particular reason to revert the backport of 5aaf179 (ec3b2fb). That commit simply changed the whitespace in the unit preview pane (see screenshots in the spoiler). That change is reasonable (which is not the same as saying I agree with it aesthetically - I don't - but that's neither here nor there), and could make sense for 1.14 independently of the damage type and range icons.

However, because Vultraz's change and the damage type and range icon changes conflict with each other, what I recommend to do is two steps:

  1. Revert 967d979, ec3b2fb, 26a30fc, and 0db0b8d (in this order, so there'll be no conflicts).
  2. Re-backport 5aaf179.

This way, Vultraz's change stays in but the other two commits stay out. (It bears mentioning that when those two commits were added to master, they were intentionally not backported, but nobody told Vultraz that was the case.)

screenshots of Vultraz's change on master

2019-09-29-121342_1920x1080_scrot 2019-09-29-121323_1920x1080_scrot

edit: Added 967d979 to the instructions

@jostephd

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

@Vultraz Looking at the screenshots in my last post, does the vertical placement of the red/black icons look right to you? Previously the layout had the red/black icons vertically aligned with a letter that had both an ascender and a descender, but now, the bottom of the icon is aligned with the baseline of the text (the horizontal stroke of the 2 in the first attack), so the top of the icon is much higher than the top end of letters with ascenders.

Vultraz added a commit that referenced this issue Sep 30, 2019
…ne (fixes #4406)

[ci skip]

See also #4394.
@Pentarctagon Pentarctagon added Blocker and removed Blocker labels Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.