After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710238 - Fix the margin in RTL
Fix the margin in RTL
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-10-16 07:23 UTC by Yosef Or Boczko
Modified: 2013-11-15 01:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GtkWidget: Add margin-start and margin-end properties (9.48 KB, patch)
2013-10-16 19:26 UTC, Yosef Or Boczko
reviewed Details | Review
GtkWidget: Add margin-start and margin-end properties (9.55 KB, patch)
2013-10-18 00:10 UTC, Yosef Or Boczko
needs-work Details | Review
GtkWidget: Add margin-start and margin-end properties (10.91 KB, patch)
2013-10-28 20:59 UTC, Yosef Or Boczko
committed Details | Review
Replace all margin-left and margin-right with margin-start and margin-end (39.32 KB, patch)
2013-11-14 22:34 UTC, Yosef Or Boczko
committed Details | Review

Description Yosef Or Boczko 2013-10-16 07:23:36 UTC
The behavior of the margin-left property and the margin-right property
in RTL in wrong.

The margin-right property in LTR adds margin on right side.
In RTL I want to add this margin to the opposite direction -
to left.

The margin-left property in LTR adds margin on left side.
In RTL I want to add this margin to the opposite direction -
to right.

This mean when the developer adds a margin with margin-right,
he need to check the direction of the widget, and if it RTL - use
margin-left instead margin-right.
And vice versa.

For examle:
if (gtk_widget_get_direction (widget) == GTK_TEXT_DIR_RTL)
  gtk_widget_set_margin_left (widget, 5);
else
  gtk_widget_set_margin_right (widget, 5);

I think on two solutions:

- To change the behavior of the margin-left property and the margin-right property,
to check the direction, and to set the margin to the right direction.
margin-left will add margin on left side in LTR and margin on right side in RTL.
margin-right will add margin on right side in LTR and margin on left side in RTL.

- To add margin-start and margin-left, to handle also in RTL.
margin-start will add margin on left side in LTR and margin on right side in RTL,
margin-left will add margin on right side in LTR and margin on left side in RTL.
It requires to add also gtk_widget_{get,set}_{start,end} functions, and to recommend
(in the document) to use margin-{start,end} instead of margin-{right,left}.
Comment 1 Yosef Or Boczko 2013-10-16 19:26:51 UTC
Created attachment 257444 [details] [review]
GtkWidget: Add margin-start and margin-end properties
Comment 2 Matthias Clasen 2013-10-17 23:29:50 UTC
Review of attachment 257444 [details] [review]:

::: gtk/gtkwidget.c
@@ +14147,3 @@
+  else
+    return _gtk_widget_get_aux_info_or_defaults (widget)->margin.left;
+}

I would use an auxiliary for the value returned by _gtk_widget_get_aux_info_or_defaults here
Comment 3 Yosef Or Boczko 2013-10-18 00:10:02 UTC
Created attachment 257607 [details] [review]
GtkWidget: Add margin-start and margin-end properties
Comment 4 Benjamin Otte (Company) 2013-10-28 13:38:48 UTC
I think if we're deprecating left/right and go with start/end, we should make those the defaults in the aux struct and do the if (direction == LEFT) thing in get/set_left/right_margin.

Also, did you make sure to fix all GTK code to not use the deprecated functions (or properties in UI files)?
Comment 5 Matthias Clasen 2013-10-28 13:40:34 UTC
Review of attachment 257607 [details] [review]:

::: gtk/gtkwidget.c
@@ +14185,3 @@
+  *start = margin;
+  gtk_widget_queue_resize (widget);
+  g_object_notify (G_OBJECT (widget), "margin-start");

it would probably be technically more correct to also notify margin-left / -right, whichever it ends up to be.

@@ +14245,3 @@
+  *end = margin;
+  gtk_widget_queue_resize (widget);
+  g_object_notify (G_OBJECT (widget), "margin-end");

Same here
Comment 6 Yosef Or Boczko 2013-10-28 18:55:59 UTC
(In reply to comment #4)
> I think if we're deprecating left/right and go with start/end, we should make
> those the defaults in the aux struct and do the if (direction == LEFT) thing in
> get/set_left/right_margin.

Sure?
It will broke some margin, for example in epiphany:
https://git.gnome.org/browse/epiphany/tree/src/ephy-toolbar.c#n185

> Also, did you make sure to fix all GTK code to not use the deprecated functions
> (or properties in UI files)?

Of course.
Comment 7 Benjamin Otte (Company) 2013-10-28 19:07:26 UTC
(In reply to comment #6)
> Sure?
> It will broke some margin, for example in epiphany:
> https://git.gnome.org/browse/epiphany/tree/src/ephy-toolbar.c#n185
> 
What I mean is that there should be a variable
  aux_info->margin.start
instead of
  aux_info->margin.left
Comment 8 Yosef Or Boczko 2013-10-28 20:59:46 UTC
Created attachment 258343 [details] [review]
GtkWidget: Add margin-start and margin-end properties
Comment 9 Benjamin Otte (Company) 2013-11-14 21:12:35 UTC
Attachment 258343 [details] pushed as 9921bec - GtkWidget: Add margin-start and margin-end properties
Comment 10 Yosef Or Boczko 2013-11-14 22:34:30 UTC
Created attachment 259843 [details] [review]
Replace all margin-left and margin-right with margin-start and margin-end
Comment 11 Benjamin Otte (Company) 2013-11-15 01:58:08 UTC
Comment on attachment 259843 [details] [review]
Replace all margin-left and margin-right with margin-start and margin-end

Attachment 259843 [details] pushed as 719dd63 - Replace all margin-left and margin-right with margin-start and margin-end