GNOME Bugzilla – Bug 59799
GtkEntry should permit right-aligned text
Last modified: 2011-02-04 16:12:26 UTC
The 1.2 GtkEntry - and, as per Havoc's description, 1.3 is included - should permit permit right-aligning the text inserted into it. This hampers developers that strive to provide a consistent interface for numeric entry, as numbers are custumarily entered aligned to the right. I would like to point out that this is the current behaviour in GAL's e-text widget, and that it is very satisfactory. The current situation with 1.3 is, if the text direction is right-to-left, that the GtkEntry aligns text to the right. This behaviour could be extended to include left-to-right locales by allowing one to call set_text_alignment() on the GtkEntry. Other interfaces that offered compatible functionality would be acceptable.
You can do gtk_widget_set_direction() on the entry, to get right aligned text in LTR locales. However, the effect will not be quite what you want ... 123! entered will display as !123. So it's not really a solution. API additions will need to wait for GTK+-2.2, however.
*** Bug 68475 has been marked as a duplicate of this bug. ***
Created attachment 12815 [details] [review] Enables adjustment of text in GtkEntry (right, left and center)
Hmm, thinking about a bit more, I think this fits into the category of alignment, rather than justification. void gtk_misc_set_alignment (GtkMisc *misc, gfloat xalign, gfloat yalign); oid gtk_frame_set_label_align (GtkFrame *frame, gfloat xalign, gfloat yalign); void gtk_tree_view_column_set_alignment (GtkTreeViewColumn *tree_column, gfloat xalign); Justification in GTK+ generally refers to the alignment of multiple lines within a paragraph. As for the interaction with gtk_text_direction(), when gtk_widget_get_direction(widget) == GTK_TEXT_DIR_RTL, it should use alignment = 1 - entry->alignment. So, a float, with 0.0 == left (or for RTL, right) aligned , 1.0 == right (resp., left) aligned.
Created attachment 12837 [details] [review] Enables any alignment of text in GtkEntry
Review comments: - A temporary variable - text_width = logical_rect.width / PANGO_SCALE; would make the computation clearer. (Actually, better PANGO_PIXELS (logical_rect.width)) - The math looks right, but it took me 5-10 minutes to verify that. I don't think the MIN/MAX approach scales for clarity. I would suggest (untested) if (gtk_widget_get_direction (GTK_WIDGET (entry)) == GTK_TEXT_DIR_LTR) alignment = entry->alignment else alignment = 1.0 - entry->alignment; text_width = PANGO_PIXELS (logical_rect.width); if (text_width > text_area_width) { min_offset = 0; max_offset = text_width - text_area_width; } else { min_offset = - (text_area_width - text_width) * alignment; max_offset = min_offset; } Which more or less obviously correct. - Need a property (and notification on the property when calling the setter) - Needs real documentation comments "Sets the alignment for the entry." Doc comment should say something that isn't just a repetition of the function name, or why have doc comments? What does the alignment mean? What are the possible values? - Need the appropriate g_return_[val_]if_fail guards. - Needs coding style fixage - see pango/docs/TEXT/coding-style for a overview that basically is the same as the GTK+ coding style. (Main difference is that GLib/GTK+ use gint not int)
I think your handling of text direction RTL is wrong. It is scrooling text of the wrong end.
I haven't tested it, so if it is actually behaving wrong on testing, well, then it's wrong. But if you look at the expression in gtkentry.c currently, you'll see that when text_width > text_area_width, you get min_offset = 0 max_offset = text_width - text_area_width for both LTR and RTL. And this makes sense - a negative offset corresponds to white space at the left end of the entry, which you shouldn't get if there is scrolling, no matter whether you are in LTR or RTL mode.
I think my brain was still thinking of the else-branch as being the RTL-case. Perhaps we could make it even clearer by removing the min_offset and max_offset and set the entry->scroll_offset directly. I can't really see that the CLAMP is doing anything good. But just as in my original change which only changed two lines, I'm in general changing as little as possible in the code, as there might be things as the original code-writer has thought about that I may mis if I re-write the code. My re-written proposal would then be: text_width = PANGO_PIXELS (logical_rect.width); if (gtk_widget_get_direction (GTK_WIDGET (entry)) == GTK_TEXT_DIR_LTR) { /* Text direction: Left to right */ if (text_width <= text_area_width) { entry->scroll_offset = (text_width - text_area_width) * entry->alignment; } else { entry->scroll_offset = text_width - text_area_width; } } else { /* Text direction: Right to left */ if (text_width <= text_area_width) { entry->scroll_offset = (text_width - text_area_width) * (1.0 - entry->alignment); } else { entry->scroll_offset = 0; } }
The clamp, is I'm pretty sure necessary. I suspect that with your patch, the effect will be that the entry is "spring-loaded" - it won't scroll when the cursor gets to one end of the entry or the other, rather it will scroll back immediately when the user isn't at the end of the entry. The basic idea of the min_offset and max_offset is that we should only change the scroll_offset when we _have_ to change the scroll offset.
I think this just shows the old knowledge - don't change things that are working :-) I did of course forget that the cursor do not need to be at the end of the text. Back in november/december this was all clear to me, but there has been so much going on since. Hopefully someone can complete the implementation, as I know I won't get time for this in the near future :-(
*** Bug 135177 has been marked as a duplicate of this bug. ***
I compared the solutions of Egon's patch (59799) with the solutuon I wrote (135177) and Egon's solution seems more compact and elegant. While I was computing a position of the layout depending on the alignment/justification, he simply changed the scroll_offset. This needs less changes and looks more elegant than mine. I therefore vote for giving up my solution (135177) in favor for Egon's (59799). I am willing to updated his changes according to the code review posted by Owen. Before I am starting doing this I once want to discuss again with Owen and others the topic of justification (enum of left, right, center) versus alignment (a float in [0;1]). I strongly vote for using justification instead of alignment because of the following reasons: o Alignment as described in GtkMisc does 'enable the widget to be positioned within its allocated area. Note that if the widget is added to a container in such a way that it expands automatically to fill its allocated area, the alignment settings will not alter the widgets position.' This means that the whole GtkEntry (including borders?) should be aligned, not just the text inside the GtkEntry. o Alignment means x and y alignment, but we only provide x alignment. And I do not see any sense for a y alignment, the height of the GtkEntry should always be such that only one line of text fits in there. o People rather think of justification than of alignment. The first version of the patch here (59799) as well as in the one I started (135177) used justification and not alignment. This indicates that people accept justification rather than alignment. o Pango also only has left, right and center justification (but uses the term alignment for this). o I don't see any sense in having a GtkEntry aligned with a value of e.g. 0.1. My guess it will look just strange. For a label I can imaging to have it aligned with a value of 0.1 (so mainly left but with some extra space if available). But in a text editor, you only have left, right and center justification. o The only good point about alignment is that it is more general than justification, but that should not mean that we should favor it. Gtk is strong in its concepts and we should think well about extensions such that they make really sense. o alignment needs a gfloat to be added to the struct GtkEntry increasing the size by 4? bytes, justification only needs 2 bits which come for free as we can include them in the bitfield already present (so the size stays same). Now, I would like to see what others think, and once we made our conclusion about this topic, I can start doing the patch which hopefully will make it into the next gtk version. -- Steffen
Thanks for the comments, but they don't convince me. Consistency with GtkLabel is the most important thing here, and we have enough problems with people trying to use justification for GtkLabel instead of alignment without having them say "But for GtkEntry, I used justification!". Another strong point in favor of alignment is that the RTL flipping is much more natural with the numeric alignment value.
I have no real problem with alignment being used here. It's certainly "feels" more consistent when you take into account RTL and LTR text in the widget.
Created attachment 24958 [details] [review] patch for gtkentry for having text aligned, includes changes according to Owen's comments
So shall it be alignment then. I just added a new patch for your review. Some comments: I moved the alignment field to the end of struct GtkEntry (out of the public area) and renamed it to xalign. I called the property 'xalign' similar to other classes (GtkMisc for example). However, the methods for setting and getting the alignment are called gtk_entry_set_alignment and gtk_entry_get_alignment. I wonder if they should be called differently or if they should take two (and return two) arguments like the other classes do? There is GtkFrame which also alows an yalign but ignores the value currently. Let me know and I can change it accordingly. -- Steffen
Applied with various fixups. (API freeze is tomorrow, so I'd like to just get it in at this point.) * moved xalign into instance-private-data. Adding fields to the end of the structure breaks binary compatibility. * Indentation - in function definitions the return type goes on a separate line. * Use P_() for property name/description translation. This allows translators to separate out user visible strings. * Changed the property description to: P_("The horizontal alignment, from 0 (left) to 1 (right). Reversed fo\r RTL layouts.") which I think makes things more obvious. * No need to freeze/thaw the notify queue when only notifying on one property. * Instead of calling gtk_widget_queue_draw(), call gtk_entry_recompute(), since simply redrawing won't changing the scroll offset. * Extend the gtk_entry_set_alignment() I think the single parameter gtk_entry_set_alignment() works well and see no reason to change it. Sun Feb 29 22:01:49 2004 Owen Taylor <otaylor@redhat.com> * gtk/gtkentry.[ch]: Add gtk_entry_set_alignment() to allow right-aligned entries and a "xalign" property. (#59799, patch from Egon Andersen and Steffen Gutmann)