Page MenuHomeWildfire Games

Fix Olist text wrapping
ClosedPublic

Authored by gentz on Dec 29 2018, 10:53 PM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22066: Fix cell text wrapping inside COList.
Summary

Currently, olist wraps it's columns when the text contained in them is greater than the size of the whole olist. So if a column has a 50% width, it wont wrap until half the text has been clipped off.

I think pictures demonstrate this the best:
Before:


After:

Test Plan

I looked at it and saw if it was wrapping.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
gentz marked 5 inline comments as done.Dec 30 2018, 1:27 PM

I've been doing svn diff COList.cpp | arc diff --raw, because I didin't want to pull in my other changes to the different parts of the code base.

COList.cpp
76 ↗(On Diff #7134)

Heh, my rust is leaking in.

gentz updated this revision to Diff 7135.Dec 30 2018, 1:27 PM
gentz marked an inline comment as done.
gentz updated this revision to Diff 7136.Dec 30 2018, 1:35 PM

And now I have to manually edit the patches... (via svn diff COList.* ../../binaries/data/mods/mod/gui/common/modern/styles.xml ../../binaries/data/mods/mod/gui/gui.* | vipe | arc diff --raw)

Currently, olist wraps it's columns when the text contained in them is greater than the size of the whole olist

You mean COList only wraps when the text is large than the size of the inherited CList?
There are a number of issues that came from inheriting CList in COList (for example the list and list_data property being required by COList but only because CList requires it, not because it is used by COList) , I guess that's one more.

Not sure about the removal of the headerheight property. I guess the setting is not so important, because noone wants the column header height to be lower than the textsize, nor does anyone reasonably want wasted space there.
But consider that the column sorting sprites also need to be accounted for.

The column header height changes look different from the issue of wrapping, no?

If the column wrapping is implemented, the clipping (rP16895) will never be needed and can be removed?

COList.cpp
95 ↗(On Diff #7136)

const &

gentz added a comment.Dec 30 2018, 2:22 PM
In D1717#68027, @elexis wrote:

Not sure about the removal of the headerheight property. I guess the setting is not so important, because noone wants the column header height to be lower than the textsize, nor does anyone reasonably want wasted space there.
But consider that the column sorting sprites also need to be accounted for.

Will do.

The column header height changes look different from the issue of wrapping, no?

Well, sort've. Previously, the headers text would just spill over into the next, but after the earlier version of the patch, the parts that would've spilled over get wrapped then clipped.
Changing the header heights stops this clipping from happening.
Of course, text will still spill over into the next column if a single word is larger than the width allocated for it's column, but that's something for another day.

If the column wrapping is implemented, the clipping (rP16895) will never be needed and can be removed?

Yes.

gentz planned changes to this revision.Dec 30 2018, 2:22 PM

The context is still unavailable. Also usually a patch needs to be created from the root folder for an easy apply.

But consider that the column sorting sprites also need to be accounted for.

That's true.

If the column wrapping is implemented, the clipping (rP16895) will never be needed and can be removed?

I think it should be left. Because you may have a width less than some font symbols.

COList.cpp
97 ↗(On Diff #7136)

Wrong indentation, spaces instead of tabs.

gentz updated this revision to Diff 7138.Dec 30 2018, 2:30 PM
gentz updated this revision to Diff 7139.Dec 30 2018, 2:34 PM
gentz marked 2 inline comments as done.

The context is still unavailable. Also usually a patch needs to be created from the root folder for an easy apply.

I can't add context without dragging in changes I don't want in. Here is it done from root, however.

smiley added a subscriber: smiley.Dec 30 2018, 2:43 PM

Just output diff to a file, manually edit the thing and use web ui. That’s what I do in such situations.

vladislavbelov added inline comments.Dec 30 2018, 3:33 PM
source/gui/COList.cpp
73 ↗(On Diff #7139)

const auto& column then.

86 ↗(On Diff #7139)

Also sprites should be accounted here. The sprites size is hardcoded currently.

gentz marked an inline comment as done.Dec 30 2018, 8:51 PM
gentz added inline comments.
source/gui/COList.cpp
86 ↗(On Diff #7139)

The sprite is accounted for, as we start m_HeadingHeight at 16.

The sprite is always going to be at most 16x16px, because it gets clipped to 16x16 in the DrawSprite function on line 432:

GetGUI()->DrawSprite(*sprite, cell_id, bz + 0.1f, CRect(leftTopCorner + CPos(width - 16, 0), leftTopCorner + CPos(width, 16)));
gentz marked an inline comment as not done.Dec 30 2018, 8:51 PM
vladislavbelov added inline comments.Dec 30 2018, 9:26 PM
source/gui/COList.cpp
86 ↗(On Diff #7139)

Yeah, I meant to move the size up to use a common constant instead of the duplication.

gentz updated this revision to Diff 7159.Dec 31 2018, 1:17 AM
gentz updated this revision to Diff 7160.Dec 31 2018, 1:18 AM
gentz marked 4 inline comments as done.
vladislavbelov added inline comments.Dec 31 2018, 2:12 AM
source/gui/COList.cpp
432 ↗(On Diff #7160)

So another 16 should be replaced here too.

source/gui/COList.h
73 ↗(On Diff #7160)

The sprite is rendered as a square, so it needs to be called similar. Also it doesn't have to be in the header, because we don't share it.

gentz added inline comments.Dec 31 2018, 2:38 AM
source/gui/COList.cpp
432 ↗(On Diff #7160)

That other 16 is for the width.

source/gui/COList.h
73 ↗(On Diff #7160)

I've moved it to the source file, but I don't get what you mean by caching.

gentz updated this revision to Diff 7161.Dec 31 2018, 2:39 AM
vladislavbelov added inline comments.Dec 31 2018, 3:02 AM
source/gui/COList.cpp
432 ↗(On Diff #7160)

Yes, but I said above, the sprite is square, so 16 is both width and height currently. So it should be defined as one constant.

source/gui/COList.h
73 ↗(On Diff #7160)

I said sharing. If you want to change a constant in a header, you need to recompile all targets that use the header. If the constant is in the source, you don't need to do that.

gentz updated this revision to Diff 7162.Dec 31 2018, 3:32 AM

I think I have no questions more. So I'm going to test it again and commit if it works.

I didn't find your name in credits (wasn't you added yet): are your credits in the D953 are up to date?

gentz added a comment.Dec 31 2018, 4:22 AM

I didn't find your name in credits (wasn't you added yet): are your credits in the D953 are up to date?

They are up to date.

One could also make it a COList setting whether or not to use linewrapping. Could even make it a config option for the replay menu, but I'm not sure if it's useful enough (as it costs some user attention time / GUI space).

If linewrapping is enabled (whether forced or not), then one must also consider the possibility for an item producing so much height that the COList becomes unusable.
Perhaps the number of characters per cell could be limited to account for that.
For example the lobby gamelist could contain games whose gamename has practically arbitrary length (ticket somewhere).

I think it should be left. Because you may have a width less than some font symbols.

As long as it's clear to the reader how the code is not dead code.
If this is the only use case it should be mentioned in a code comment (otherwise if it's optional the use case would be self-explanatory).

auto

We avoid auto, so that the reader is informed of the type of each variable without having to lookup the declaration. Though we do use the typedefs if they are declared.

heading_height removal

Perhaps that could also become min/max height? (I know, additional work, but we have to consider all use cases of COList before we limit the versatility of the GUI object type)

scope

{ } can be removed if the scope contains only one statement

source/gui/COList.cpp
1 ↗(On Diff #7162)

9

26 ↗(On Diff #7162)

Is auto-resizing the column header to the content dimensions really important?
If so, it would be consistent feature-wise and code-wise to do this equally for images and text.
There would be no real reason to force 16*16 sprites if columns are autoresized depending on content.

In D1717#68290, @elexis wrote:

As long as it's clear to the reader how the code is not dead code.
If this is the only use case it should be mentioned in a code comment (otherwise if it's optional the use case would be self-explanatory).

Do you suggest to add it here?

We avoid auto, so that the reader is informed of the type of each variable without having to lookup the declaration. Though we do use the typedefs if they are declared.

We can use auto in some cases, e.g. iterators.

Perhaps that could also become min/max height? (I know, additional work, but we have to consider all use cases of COList before we limit the versatility of the GUI object type)

Does it make sense for headers? Usually a whole header text is important, because it describes a column meaning.

source/gui/COList.cpp
26 ↗(On Diff #7162)

I didn't get what do you mean.

Auto-resizing is important, because we can have long column names.

We can't process images and texts equally yet because they have different infrastructures.

The sprite size is fixed, because we don't have methods to get a real sprite size, because it could be drawn with different sizes.

In D1717#68290, @elexis wrote:

One could also make it a COList setting whether or not to use linewrapping. Could even make it a config option for the replay menu, but I'm not sure if it's useful enough (as it costs some user attention time / GUI space).
If linewrapping is enabled (whether forced or not), then one must also consider the possibility for an item producing so much height that the COList becomes unusable.
Perhaps the number of characters per cell could be limited to account for that.
For example the lobby gamelist could contain games whose gamename has practically arbitrary length (ticket somewhere).

Isn't this sort've unrelated to this PR? Linewrapping has always been enabled for the items. It's just that prior to these changes, it would start wrapping too late.

The only thing I've added line wrapping to is the column headers themselves.

scope

{ } can be removed if the scope contains only one statement

Where?

gentz updated this revision to Diff 7168.Dec 31 2018, 11:58 PM
gentz marked an inline comment as done.
gentz updated this revision to Diff 7169.Jan 1 2019, 12:09 AM
Stan added inline comments.Jan 1 2019, 9:49 AM
source/gui/COList.cpp
79 ↗(On Diff #7169)

There I guess.

102 ↗(On Diff #7169)

Here also

gentz updated this revision to Diff 7175.Jan 1 2019, 11:51 AM
gentz marked 9 inline comments as done.

@elexis do you have any objections to commit it?

elexis added a comment.Jan 1 2019, 6:12 PM

@elexis do you have any objections to commit it?

I think it would be healthy to derive any consequences from the implementation and rule out any undesired or unintended effects and to check if this doesn't put any unneeded limitations to OList users before committing something.
This doesn't have to be done by 5 people, but at one person should have checked exhaustively (as in exhausting the evidence, not the person).

We can use auto in some cases, e.g. iterators.

That's what the CC says, but I don't see the reasoning behind that.
If auto is bad because it covers up meaning, then why would it be good to cover up the meaning of iterators?
It's not hard to use the right type and it makes the lines a bit more transparent that use the iterator.
(Regardless whether this patch uses iterators or not)

Do you suggest to add it here?

The requirement is that it should always be clear to the reader why present code is not dead code.
So two choices:

  1. If the code only exists to address an unpredictable edge case (such as column width smaller than one character), mention that edge case.
  2. Examine if there is more value in the code than addressing an extreme edge case.

Does it make sense for headers? Usually a whole header text is important, because it describes a column meaning.

Autoresizing is a good addition, but are there any possible consequences of the implementation as is that might yield unintended effects?
Is forcing this auto-resizing good in any case?
One danger with auto-resizing is when there are extreme edge cases.
If there is one column with a small width but large content, then the column header will become very large.
The column header values contain translations, so the case can't really be avoided unless with a fixed column height or maxHeight.
The same problem can be applied to cells.
For some players or GUI modders it may be undesirable to enable linewrapping to begin with.

Linewrapping has always been enabled for the items. It's just that prior to these changes, it would start wrapping too late.

I have never seen linewrapping for COList. Maybe this is just a bugfix, but it is a behavior change that significantly changes how the lobby, savegame, modselections and replaymenu appear, so we should make sure it's the right way to go.
Just because the previous code had some features doesn't mean that they were always planned like that or the best/most versatile implementation.

source/gui/COList.cpp
26 ↗(On Diff #7162)

We can't process images and texts equally yet because they have different infrastructures.

Pixels?

we don't have methods to get a real sprite size

m_Size

The sprite (not the imagefile) has a size property which should be used. Maybe doesn't have to be in this commit and this commit does it cleaner than what was here before.
But hardcoding a fixed size violates the definition of the size property of the sprite, so different code from this would be the correct implementation. That aspect should be worked out and then one can commit this as a step towards it, not a step in spite of that.

In D1717#68419, @elexis wrote:

We can use auto in some cases, e.g. iterators.

That's what the CC says, but I don't see the reasoning behind that.
If auto is bad because it covers up meaning, then why would it be good to cover up the meaning of iterators?
It's not hard to use the right type and it makes the lines a bit more transparent that use the iterator.

I understand you point, it's best practice for most cases. Why auto is good enough for iterators, because it's pretty standard to write auto it = container.begin(); for long types instead of very_long_container_type::iterator it = container.begin();. We can do this because it's a standard for that case. E.g. we don't use signed for int or char, but we can and it makes things more obvious.

Do you suggest to add it here?

The requirement is that it should always be clear to the reader why present code is not dead code.
So two choices:

  1. If the code only exists to address an unpredictable edge case (such as column width smaller than one character), mention that edge case.
  2. Examine if there is more value in the code than addressing an extreme edge case.

I forgot, we don't split a word by symbols, only words by delimiters like spaces, tab or special characters. So we have to clip columns and we have to leave the clipping code, because its meaning wasn't changed.

Autoresizing is a good addition, but are there any possible consequences of the implementation as is that might yield unintended effects?
Is forcing this auto-resizing good in any case?
One danger with auto-resizing is when there are extreme edge cases.
If there is one column with a small width but large content, then the column header will become very large.
The column header values contain translations, so the case can't really be avoided unless with a fixed column height or maxHeight.
The same problem can be applied to cells.
For some players or GUI modders it may be undesirable to enable linewrapping to begin with.

My point is that we don't loose something here except fixed heading height. And it's not bad, because usage of short and capacious names for columns is the good practice. And even if we have a long translation, it'd be possible to read the whole column name on all languages.

Linewrapping has always been enabled for the items. It's just that prior to these changes, it would start wrapping too late.

I have never seen linewrapping for COList. Maybe this is just a bugfix, but it is a behavior change that significantly changes how the lobby, savegame, modselections and replaymenu appear, so we should make sure it's the right way to go.
Just because the previous code had some features doesn't mean that they were always planned like that or the best/most versatile implementation.

The linewrapping was enabled for long time, it just had a wrong width. So we don't have a regression here.

source/gui/COList.cpp
26 ↗(On Diff #7162)

Pixels?

No, we layout texts and images separately. We can't insert images into a text directly. By together layout I mean layout like in HTML or inside our XML. We have the hardcoded layout in our GUI elements.

m_Size

Nope, CGUISprite has it, but we have only CGUISpriteInstance.

I tested the patch and it works good, except the inline comment.

source/gui/COList.cpp
86 ↗(On Diff #7175)

We forgot about the hardcoded shift for the column name CPos(0, 4).

The linewrapping was enabled for long time, it just had a wrong width. So we don't have a regression here.

It's a non sequitur (Just because something was there before and hidden by a bug doesn't mean that removing the bug and making the feature visible is the only right response.
I'm saying all consequences of the patch should be examined to identify possibly unintended side-effects, such as using line-wrapping when a user (GUI creator or player) wants to avoid linewrapping, or column or cell heights becoming so large that it becomes ugly or less usable.)

Extreme cell/column heights should indeed be an unlikely situation, but for the cell values it's easy that there could be a line that contains an extremely long user strings.
COList would probably be more versatile and safer if there was a linewrapping, linewrapping_column bool and max_height, max_column_height int setting.
(That doesn't have to be in this patch as mentioned, but the feature design review should be part of a review.)

source/gui/COList.cpp
26 ↗(On Diff #7162)

We can't insert images into a text directly

Not necessary to do so either. The code can remain the same, just the hardcoding can be replaced with the user-specified size. Even relative sizes shouldn't be a problem because m_CachedActualSize is known.

CGUISprite has it, but we have only CGUISpriteInstance

and m_Sprites stores the CGUISprite that the CGUISpriteInstance refers to, which could be exposed through a getter for instance.

In D1717#68712, @elexis wrote:

The linewrapping was enabled for long time, it just had a wrong width. So we don't have a regression here.

It's a non sequitur (Just because something was there before and hidden by a bug.

It wan't hidden, just a threshold of line-wrapping was bigger.

Extreme cell/column heights should indeed be an unlikely situation, but for the cell values it's easy that there could be a line that contains an extremely long user strings.
COList would probably be more versatile and safer if there was a linewrapping, linewrapping_column bool and max_height, max_column_height int setting.

It makes sense. But it'd be good to have a real usecase. When someone really wans it.

source/gui/COList.cpp
26 ↗(On Diff #7162)

CGUISprite isn't the one sprite, but the set. So we need to duplicate logic here then. Or somehow unify it.

elexis added a comment.Jan 3 2019, 4:25 PM

It wan't hidden, just a threshold of line-wrapping was bigger.

I have never seen it in action, so it was invisible to me (even if the C++ property hidden was set to true) and I'm not sure if anyone else has ever seen it (lobby gamelist and replay playerlist values are currently not limited in length afaik)?

It makes sense. But it'd be good to have a real usecase. When someone really wans it.

Notice some lists are also updated frequently - sometimes a cell might have a short value, other times a cell might have a long value, resulting in a flickering of cell heights.
There might be instances that one could derive from the code, some far-sightedness and/or testing or committing and having thousands of players test it and find the weird edge cases.
To answer the question, no, I don't see anything to raise a concern for yet and it's probably a good addition - thanks for the patch and the review. If there is some catch 22, we can still claim to should have known better afterwards.

source/gui/COList.cpp
26 ↗(On Diff #7162)

So the COList code also breaks the definition of a sprite in the way that a sprite consists of N images, not only one?
Shouldn't be too complicated to handle N images instead of 1 image in both cases. But I guess that can be done on the day after doomsday or written into a trac ticket too or left as self-evident to the source/GUI/ maintainers.

I have never seen it in action, so it was invisible to me (even if the C++ property hidden was set to true) and I'm not sure if anyone else has ever seen it (lobby gamelist and replay playerlist values are currently not limited in length afaik)?

It's visible if a cell text is longer than the COList width.

source/gui/COList.cpp
26 ↗(On Diff #7162)

So the COList code also breaks the definition of a sprite in the way that a sprite consists of N images, not only one?

No, COList uses fixed size, so no problem here.

elexis added a comment.Jan 3 2019, 5:39 PM

It's visible if a cell text is longer than the COList width.

Okay, you are right, it just was luck that we didn't see it before.
I thought such cases would have been tested already by the most dedicated trolls.

source/gui/COList.cpp
26 ↗(On Diff #7162)

To the confusion and disappointment of the user who had configured a different size or position.
If the size is set into stone in COList, then it should not be possible to specify a custom size (at least if someone cares about the benefits of APIs that work when they are exposed and XML validation.) (Settings should exist if and only if they have an effect.) (As I said, doesn't have to be solved in this diff, or anytime soon either, it's just a deviance from the ideal)

elexis awarded a token.Jan 3 2019, 5:39 PM

So, the patch is good for me, but the CPos(0, 4) should be accounted.

gentz updated this revision to Diff 7231.Jan 4 2019, 2:06 AM
elexis added inline comments.Jan 5 2019, 4:21 PM
source/gui/COList.cpp
26 ↗(On Diff #7162)

We can't insert images into a text directly

(Also one can, for example setup_resources.xml. But those have variable size too)

@elexis Do you have any objection that prevents to commit the patch?

https://code.wildfiregames.com/D1717#68827 (also that wall of text about committing things)

In D1717#70498, @elexis wrote:

https://code.wildfiregames.com/D1717#68827 (also that wall of text about committing things)

I guess you agree, so I just confirm that.

vladislavbelov accepted this revision.Jan 24 2019, 8:54 PM

I tested the patch, it works and looks good for me. In any case it's the improvement. If someone wants a more flexible header, the one can create a ticket or ask for a patch.

This revision is now accepted and ready to land.Jan 24 2019, 8:54 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 24 2019, 9:01 PM

@gentz thank you for the patch!