Opened 4 years ago

Closed 4 years ago

#10544 closed defect (fixed)

CP: Switch to GNOME Button not displayed correctly in all the languages

Reported by: erikos Owned by: erikos
Priority: normal Milestone: 11.2.0-M4
Component: sugar Version: Development build as of this date
Keywords: Cc: dsd, godiard
Blocked By: Blocking:
Deployments affected: Action Needed: no action
Verified: no

Description

Reproduce:

  • change your language to be French
  • go to CP and try to switch to GNOME

Attachments (17)

Capture d'ecran.png (149.6 KB) - added by erikos 4 years ago.
Screenshot of the issue
0001-Increase-width-requested-to-the-labels.patch (2.9 KB) - added by godiard 4 years ago.
0001-Add-scrolled-window-to-the-langueges-where-the-text-.patch (5.0 KB) - added by godiard 4 years ago.
0001-Increase-width-requested-to-the-labels-V2.patch (3.0 KB) - added by godiard 4 years ago.
text_with_window_width spanish ERROR.png (140.6 KB) - added by godiard 4 years ago.
3_2 screen arabe.png (147.0 KB) - added by godiard 4 years ago.
3_2 screen english.png (146.9 KB) - added by godiard 4 years ago.
3_2 screen french.png (149.0 KB) - added by godiard 4 years ago.
3_2 screen portugues.png (149.5 KB) - added by godiard 4 years ago.
3_2 screen spanish.png (147.1 KB) - added by godiard 4 years ago.
scrolled_window_arabic.png (147.8 KB) - added by godiard 4 years ago.
scrolled_window_english.png (149.1 KB) - added by godiard 4 years ago.
scrolled_window_french.png (152.4 KB) - added by godiard 4 years ago.
scrolled_window_portuguese.png (134.6 KB) - added by godiard 4 years ago.
scrolled_window_spanish.png (148.4 KB) - added by godiard 4 years ago.
view.py (7.2 KB) - added by godiard 4 years ago.
0001-The-labels-are-justified-and-all-the-content-in-the-.patch (5.7 KB) - added by godiard 4 years ago.

Download all attachments as: .zip

Change History (46)

Changed 4 years ago by erikos

Screenshot of the issue

comment:1 Changed 4 years ago by godiard

  • Action Needed changed from code to review

Attach a solution proposal.

comment:2 Changed 4 years ago by godiard

  • Cc dsd added

The first patch used a fixed size of 2/3 of the window instead of 1/2 of the window to the text.

This is not a good solution.

Talked with erikos and now I added a scrolled window. The second patch replace the first.

comment:3 follow-up: Changed 4 years ago by dsd

have you tested it with a variety of languages?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by erikos

Replying to dsd:

have you tested it with a variety of languages?

Ok, I tested some languages and so far only French seem to have the issue. The simplest fix for 10.1.3 would be to just do:

self._gnome_opt_label.set_size_request(gtk.gdk.screen_width(), -1)

instead of:

self._gnome_opt_label.set_size_request(gtk.gdk.screen_width() / 2, -1)

And for the next build we do the scrolling window. But I am ok with moving it out, if others are not convinced it is worth it.

comment:5 Changed 4 years ago by godiard

  • Cc godiard added

comment:6 Changed 4 years ago by dsd

I think it's been decided that it's too late for 10.1.3 (which I agree with).

still want the fix though! patch looks good but my question is whether it has been tested. in addition to fixing french, it should be tested to ensure that there is no change for English, Spanish, and another 2 languages of your testing choice.

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

comment:7 in reply to: ↑ 4 Changed 4 years ago by godiard

Replying to erikos:

Replying to dsd:

have you tested it with a variety of languages?

Ok, I tested some languages and so far only French seem to have the issue. The simplest fix for 10.1.3 would be to just do:

self._gnome_opt_label.set_size_request(gtk.gdk.screen_width(), -1)

instead of:

self._gnome_opt_label.set_size_request(gtk.gdk.screen_width() / 2, -1)

And for the next build we do the scrolling window. But I am ok with moving it out, if others are not convinced it is worth it.

I have tested it but does not work (see http://dev.laptop.org/attachment/ticket/10544/text_with_window_width%20spanish%20ERROR.png)

gtk.gdk.screen_width() return the screen width in pixels, not the popup width.

I tried again width 2 / 3 of screen . Is a magic number but 1 / 2 of screen too.

I have tested it in Spanish, French, English, Arabic and Portuguese.

0001-Increase-width-requested-to-the-labels-V2.patch add a variable width to change it in one place.

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

Changed 4 years ago by godiard

comment:8 Changed 4 years ago by godiard

I have attached screenshots with the scrolled window patch applied in Arabic, English, French, Portuguese and Spanish.

comment:9 Changed 4 years ago by dsd

So which patch are you proposing? And did you see any errors in the languages tested?
It sounds like using gtk.gdk.screen_width() is wrong, but it's not clear if you are proposing a solution that avoids that.

comment:10 Changed 4 years ago by godiard

Daniel, I think the two solutions work, but i don't know what is the best solution at this moment.

I have talked with erikos and he say 2/3 of the screen is a magic number, and i agree, but I don't know if you want a change more invasive like adding a ScrolledWindow now.

comment:11 follow-up: Changed 4 years ago by dsd

No problem with invasive changes, lets fix this properly. I think we already decided that this is not 10.1.3 material.

comment:12 in reply to: ↑ 11 Changed 4 years ago by godiard

Replying to dsd:

No problem with invasive changes, lets fix this properly. I think we already decided that this is not 10.1.3 material.

I don't think so. erikos said "The simplest fix for 10.1.3 would be..."

I think we need a fix for 10.1.3

comment:13 Changed 4 years ago by erikos

  • Action Needed changed from review to package
  • Milestone changed from 10.1.3 to F14

Let's move this to the next milestone.

comment:14 follow-up: Changed 4 years ago by martin.langhoff

Is there no low-risk way to workaround or fix this issue? Can we ask Sam to take a patched version and see if anything breaks...?

comment:15 Changed 4 years ago by dsd

I don't think we've identified any "low-risk" fixes right now - as this bug shows just how sensitive the layout is.

Given that this is a minor bug, has a workaround, is not a regression, does not affect any large deployments, and that 10.1.3 is already overdue, I think it should be pushed out.

comment:16 in reply to: ↑ 14 Changed 4 years ago by godiard

Replying to martin.langhoff:

Is there no low-risk way to workaround or fix this issue? Can we ask Sam to take a patched version and see if anything breaks...?

Low-risk solution to check by Sam: copy the view.py file in /usr/share/sugar/extensions/cpsection/switchdesktop/

Anyway, dsd will not include any solution in 10.1.3

Changed 4 years ago by godiard

comment:17 Changed 4 years ago by erikos

Let's make sure we get the right fix into the next F14 build, we might get new dance material then:
http://www.dailymotion.com/video/xftdx1_clip-nosy-komba_tech

comment:18 Changed 4 years ago by dsd

Yeah. What I'd like to see next is clarity on a patch being proposed, and written indication that testing has been performed with no problems found. Gonzalo pointed out on IRC that the existing layout code isn't very good and could do with an overhaul - I'd welcome an invasive patch to fix this properly.

comment:19 Changed 4 years ago by godiard

Daniel:

I propose a new patch. This time i removed all the size request in the labels, and justified the texts.

I have added a ScrolledWindow for the cases when the text is longer than the place you can provide in the window.

The justification does not work very well with labels with line wrap, but I tried many different combination and that is the better.

I have tested it with many languages and found no problems.

comment:20 follow-up: Changed 4 years ago by dsd

Thanks, looking good.

label_space looks unused though.

Also, the justification has not been fully maintained. "Sugar is the" is no longer aligned with "You can return" on the main screen, and after choosing to switch, "Restart your computer to" is not aligned with "Remember, you can return"

comment:21 in reply to: ↑ 20 Changed 4 years ago by godiard

Replying to dsd:

Thanks, looking good.

label_space looks unused though.

It's true. My mistake.

Also, the justification has not been fully maintained. "Sugar is the" is no longer aligned with "You can return" on the main screen, and after choosing to switch, "Restart your computer to" is not aligned with "Remember, you can return"

The justification is in the same paragraph. The paragraphs don't have the same size, because the size is calculated by gtk.

Like the documentation says: "in reality fill, expansion, requested sizes, widget expansion semantics, container packing semantics, electron spins and lunar cycles are computed to determine how much space each widget wins. " http://faq.pygtk.org/index.py?req=show&file=faq12.002.htp

comment:22 follow-up: Changed 4 years ago by dsd

Is it possible to maintain the appearance through some other approach? In my opinion the new version looks jumbled because of this.

comment:23 in reply to: ↑ 22 Changed 4 years ago by godiard

Replying to dsd:

Is it possible to maintain the appearance through some other approach? In my opinion the new version looks jumbled because of this.

No really.

If you prefer your approach, may be is better use only a ScrolledWindow and your size request in the labels. I don't like it, but you are the maintainer. I have spent to much time with this.

comment:24 Changed 4 years ago by dsd

Simon, what are your thoughts?

comment:25 Changed 4 years ago by dsd

  • Milestone changed from 11.2.0-M1 to 11.2.0-M4

comment:26 Changed 4 years ago by erikos

I think Gonzalo's approach is ok. Another solution would be to instead of requesting a specific size of the label setting the border of the box to something that gives us nice spacing (style.GRID_CELL_SIZE) or similar.

comment:27 Changed 4 years ago by dsd

  • Action Needed changed from package to add to build

GTK+-3 seems to do a better job here, probably due to its height-for-width rendering - according to a separate test we can probably just set all widgets to have the width of the largest one.

For GTK+-2 that doesn't quite work, but telling all widgets to use the full space allocated to the container does. pushed a fix in olpc-switch-desktop-0.8 based on gonzalo's work, thanks!

comment:28 Changed 4 years ago by dsd

  • Action Needed changed from add to build to test in build

test in 11.2.0 build 19

comment:29 Changed 4 years ago by greenfeld

  • Action Needed changed from test in build to no action
  • Resolution set to fixed
  • Status changed from new to closed

Tested the two screens in the "Switch to Gnome" dialog in Arabic, English, French, Portuguese, Spanish, and a few other languages in 11.2.0 os19.

I did not see any obvious button size or text layout issues, although I am not an expert on International text layouts.

Note: See TracTickets for help on using tickets.