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

[widget audit] toga.Label #1799

Merged
merged 24 commits into from Mar 22, 2023
Merged

[widget audit] toga.Label #1799

merged 24 commits into from Mar 22, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Mar 1, 2023

A testing and documentation audit of the toga.Label widget.

Audit checklist

  • Core tests ported to pytest
  • Core tests at 100% coverage
  • Core docstrings complete, and in Sphinx format
  • Documentation complete and consistently formatted
  • cocoa backend at 100% coverage
  • winforms backend at 100% coverage
  • gtk backend at 100% coverage
  • iOS backend at 100% coverage
  • android backend at 100% coverage
  • Widget support table updated

@freakboy3742 freakboy3742 marked this pull request as ready for review March 1, 2023 05:19
else:
self.native.setJustificationMode(LineBreaker.JUSTIFICATION_MODE_NONE)
self.native.setGravity(Gravity.CENTER_VERTICAL | align(value))
self.native.setGravity(Gravity.CENTER_VERTICAL | align(value))
Copy link
Member

@mhsmith mhsmith Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous code, I wasn't sure whether setting a label to centered and then to justified would give you a result with short lines still centered. Turns out it doesn't, because the native widget's horizontal gravity defaults to LEFT if unspecified, but this isn't clearly documented, so it's better to be explicit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow what you're saying here. Isn't this setting horizontal gravity every time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now, but previously it was leaving the horizontal gravity unspecified when value == JUSTIFY.

@mhsmith
Copy link
Member

mhsmith commented Mar 21, 2023

There are still a few problems with newlines in labels in iOS, which you can test interactively with examples/resize:

First, I found that although clicking the H button always increased the widget height, about half of the time it didn't display the additional line, but just added extra space above and below the existing lines. For example, here's what happened when increasing the height from 1 to 2:

I fixed this by rounding the hinted dimensions up to the next integer, but do you know how this could be tested? We'd need some way to determine which part of the label text is actually visible.

@mhsmith
Copy link
Member

mhsmith commented Mar 21, 2023

Also visible in examples/resize on iOS:

  • When a label has multiple lines, it sometimes sets the label width to less than the maximum line width and wraps the text, which the other platforms don't do. I tried different combinations of numberOfLines = 0 and lineBreakMode = NSLineBreakByClipping, but they all still wrap in some cases. It seems to depend on what size the widget was previously, in a way that I don't fully understand.

    This may be explained by the systemLayoutSizeFittingSize documentation, which says it returns "the optimal size of the view based on its current constraints". Maybe we need to remove those constraints before calling the method?

  • An empty label on iOS has a height of zero, whereas the other platforms would give it as the height of a single line. I've added a test to expose this, but I haven't started working on a fix yet. It could probably be done with a zero-width space, as with the Winforms Button.

@freakboy3742
Copy link
Member Author

Also visible in examples/resize is that empty labels collapse into nothing on iOS, where the other platforms display them as the height of a single line. I've added a test to expose this, but I haven't started working on a fix yet. It could probably be done with a zero-width space, as with the Winforms Button.

Yeah - looks like a ZWS is the best approach here; I've implemented that, and also updated the implementation to not use a shadow variable in the interface.

I think the behavior on the resize app is now correct (or, at least, is arguably correct) - on macOS, the "empty" label renders with a tiny, but nonzero width; iOS doesn't have any width, but it does have a height, AFAICT. If you think there's value in having some visible appearance, it's easy enough to set the intrinsic width to at_least(max(5, ceil(fitting_size.width))) so that there's always a token visible representation of a label.

I'm not sure what's going on with the line wrapping though. I can maybe see a way in which an existing layout with a short line of text that changes to a long line of text might cause a word wrap to occur... but I haven't been able to reproduce that behaviour in practice. I've modified the text examples to deliberately increase and decrease sizes, but I don't think that's enough to exercise the class of problem you're talking about. Have you got an explicit example that demonstrates this?

@mhsmith
Copy link
Member

mhsmith commented Mar 22, 2023

on macOS, the "empty" label renders with a tiny, but nonzero width; iOS doesn't have any width, but it does have a height, AFAICT. If you think there's value in having some visible appearance, it's easy enough to set the intrinsic width to at_least(max(5, ceil(fitting_size.width))) so that there's always a token visible representation of a label.

I think this is the kind of minor difference between platforms which we can live with for now.

@mhsmith
Copy link
Member

mhsmith commented Mar 22, 2023

I can maybe see a way in which an existing layout with a short line of text that changes to a long line of text might cause a word wrap to occur... but I haven't been able to reproduce that behaviour in practice.

It could be reproduced in examples/resize as follows:

  • Set the TEXT label to a width of 2 and a height of 2. Result is correct so far:
     X X
     X X
    
  • Click the W button to increase the width to 3.
  • Instead of getting wider, the label stays the same size, wraps the first line of 3 Xs across 2 lines, and doesn't display the second line at all:
     X X
     X
    

I confirm this is fixed by the most recent commit. I'll create a separate issue about testing it.

@mhsmith mhsmith merged commit a6968e8 into beeware:main Mar 22, 2023
42 checks passed
@freakboy3742 freakboy3742 deleted the audit-label branch March 22, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS doesn't support newlines in labels
2 participants