ubuntu / yaru.dart

Ubuntu Yaru Flutter widgets and themes for building desktop and web applications
https://ubuntu.github.io/yaru.dart/
Mozilla Public License 2.0
179 stars 33 forks source link

fix(YaruToggleButton): fix RTL layout #893

Closed d-loose closed 1 month ago

d-loose commented 1 month ago

Fix #690 Related issue: lp:2064700

I don't quite understand neither the original intention behind those constraints nor the very detailed assertions on the sizes of the render objects in the tests for YaruToggleButton (which will all fail for this PR). This change follows the logic of material's ListTile.

I'll test it more thoroughly next week.

d-loose commented 1 month ago

Unfortunately this seems to cause some rendering issues in other places like expansion panels :(. I don't have the patience to debug these issues on a RenderObject level right now, though.

d-loose commented 1 month ago

I'm re-opening this, since I can't reproduce the issues I had yesterday with this branch. Could someone please test this in a Flutter application of their choice that uses Yaru? :) (Ideally for both text directions)

The example app seems to work fine and the RTL issues are resolved:

Screenshot from 2024-05-07 10-59-23

yarons commented 1 month ago

I totally understand what you're saying, so the top left is correct or a mistake? The bottom left is the only part that requires more testing, you've completely nailed it with the rest of the items including the illusive spacing. Good job :)

d-loose commented 1 month ago

I'm able to reproduce the issues I had before now - it happens e.g. when you place a toggle button in a Row:

diff --git a/example/lib/pages/switch_page.dart b/example/lib/pages/switch_page.dart
index b983f865..1fd45054 100644
--- a/example/lib/pages/switch_page.dart
+++ b/example/lib/pages/switch_page.dart
@@ -39,10 +39,22 @@ class _SwitchPageState extends State<SwitchPage> {
             ],
             const Divider(),
             for (var i = 0; i < _buttonValues.length; ++i) ...[
-              YaruSwitchButton(
-                value: _buttonValues[i],
-                onChanged: (v) => setState(() => _buttonValues[i] = v),
-                title: const Text('YaruSwitchButton'),
+              Row(
+                children: [
+                  YaruSwitchButton(
+                    value: _buttonValues[i],
+                    onChanged: (v) => setState(() => _buttonValues[i] = v),
+                    title: const Text('YaruSwitchButton'),
+                  ),
+                  Container(
+                    decoration:
+                        BoxDecoration(border: Border.all(color: Colors.red)),
+                    child: const SizedBox(
+                      width: 50,
+                      height: 10,
+                    ),
+                  ),
+                ],
               ),
               const SizedBox(height: 10),
             ],

leads to the following exception when calculating the titleSize in performLayout():

════════ Exception caught by rendering library ═════════════════════════════════
The following assertion was thrown during performLayout():
BoxConstraints forces an infinite width.
These invalid constraints were provided to RenderParagraph's layout() function by the following function, which probably computed the invalid constraints in question:
  _YaruRenderToggleButton._layoutBox (package:yaru/src/widgets/yaru_toggle_button_layout.dart:177:9)
The offending constraints were: BoxConstraints(w=Infinity, 0.0<=h<=Infinity)

Also note that there's something wrong with the calculation of the title width even with LTR text direction (it just typically doesn't cause any issues because it overflows to the right).

Screenshot of the sample application with debug painting on main:

image

Jupi007 commented 1 month ago

@d-loose I also noticed this problem of overflow, I was able to fix it in my last screenshot example. It comes from taking available width in account for text constraint.

d-loose commented 1 month ago

Ah, that sounds great - does that also fix the RTL issue?

Jupi007 commented 1 month ago

Ah, that sounds great - does that also fix the RTL issue?

Yes! :D All I need to know is how the title/subtitle should be layout (expanded or not).

d-loose commented 1 month ago

Awesome! I don't have strong opinions on this, but since it's based on the ListTile implementation I guess expanded would be the most consistent choice

yarons commented 1 month ago

@Jupi007 any way I can help? From your example I think the box should have the ability yo stretch the label all the way to the width of the box with padding and possibly also ellipsis (but that's up to the developer usually).

Jupi007 commented 1 month ago

I think I found a good solution which fits all situations.

So, with an infinite width constraint (like in a Row), the title is self-sized:

Capture d’écran du 2024-05-11 23-02-11

In any other case, the title is extended to fill the available space:

Capture d’écran du 2024-05-11 23-01-57

I will update the related tests as they do not reflect this new behavior.

@yarons, if you want to help me, once I push a new PR, can you test it on apps that use these toggle button widgets to see if everything is fine?

yarons commented 1 month ago

I sure can, thank you.

d-loose commented 1 month ago

Superseded by #896