Opened 10 years ago

Closed 9 years ago

Last modified 6 years ago

#610 closed enhancement (fixed)

Journal Palette does not manage too many characters for a title correctly

Reported by: IwikiwI Owned by: tomeu
Priority: Normal Milestone:
Component: journal Version: Unspecified
Severity: Minor Keywords: right click journal sugar-love
Cc: eben, sascha_silbe, benzea Distribution/OS: Fedora
Bug Status: Assigned

Description

When right clicking on a journal item, if the name happens to be n characters long (These n being beyond screen size) the menu extends beyond the screen, and no menu items are displayed.

I have a temporary fix:

if metadata.has_key('title'):
#Hack limits to specified number of characters, and saves screen from going all the way!


screen=gtk.gdk.Screen()
width = screen.get_width()

if width >= 800 and width <= 1200:

char_length = 15


elif width > 1200 and width <= 1650:

char_length = 20


else:

char_lenghth = 25


if len(metadatatitle?)>char_length:

title = metadatatitle?[0:char_length]+"..."

else:

title = metadatatitle?

any suggestions for a good permanent fix?

Attachments (1)

palette-width.patch (1.6 KB) - added by benzea 10 years ago.
patch implementing the suggestion

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by eben

This shouldn't be left to the palette creator to manage. In fact, I thought palettes already had some code which enforced a maximum width. In any case, the palette class is where the fix should be, I think.

Also, we're not using a fixed-width font so depending on a character limit isn't robust enough. We should be trimming to fit a specific width relative to the screen size.

comment:2 Changed 10 years ago by erikos

  • Summary changed from right click on journal items to Journal Palette does not manage too many characters for a title correctly

comment:3 Changed 10 years ago by sascha_silbe

  • Cc sascha_silbe added

comment:4 Changed 10 years ago by tomeu

  • Severity changed from Minor to Blocker

comment:5 Changed 10 years ago by erikos

  • Milestone changed from Unspecified by Release Team to 0.84

comment:6 Changed 10 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned

comment:7 Changed 10 years ago by homunq

  • Keywords sugar-love added

comment:8 Changed 10 years ago by tomeu

I don't see an unintrusive way to do this properly in 0.84, so unless someone speaks, I'm just going to do the following for 0.84:

diff --git a/src/sugar/graphics/palette.py b/src/sugar/graphics/palette.py
index 919721b..1c668b6 100644
--- a/src/sugar/graphics/palette.py
+++ b/src/sugar/graphics/palette.py
@@ -145,7 +145,7 @@ class Palette(gtk.Window):
     # DEPRECATED: label is passed with the primary-text property, accel_path 
     # is set via the invoker property, and menu_after_content is not used
     def __init__(self, label=None, accel_path=None, menu_after_content=False,
-                 text_maxlen=0, **kwargs):
+                 text_maxlen=60, **kwargs):
 
         self.palette_state = self.PRIMARY

comment:9 Changed 10 years ago by IwikiwI

Sorry, I was busy with my application and stuff.
I will get back to this. Can you give me 8 hrs?

comment:10 Changed 10 years ago by IwikiwI

Eben,
So here's what I am thinking

If say 10 characters are 20 pango units. And naturally 20 characters will definitely be above 20 pango units. So we can be sloppy, and ideally limit characters by pango units. I will see if theres a method to slice a string with respect to pango units. If not long way parse each character add up, and slice then.

And if the characters are around say 8-13 then we do a check on each character width, and perform a trim.

http://www.pygtk.org/pygtk2reference/class-pangofontdescription.html#method-pangofontdescription--get-size

comment:11 Changed 10 years ago by IwikiwI

if text_maxlen > 0:

self._label.set_max_width_chars(text_maxlen)
self._label.set_ellipsize(pango.ELLIPSIZE_END)

Ok! There's already code built to slice the string when the string goes beyond the

I will just need to draw the palette frame according to the screen size, will this do?

comment:12 Changed 10 years ago by eben

I'm not sure this solves the problem. The root of the issue is that we have no way, presently, to trim the text label to a specific width. We should allow setting the min/max width/height of a palette, and adjust the strings within accordingly. The current hack allows one to trim the string to a specific number of characters, which is not what's really needed.

As a note, I think we should be using ELLIPSIZE_MIDDLE, so that we don't truncate the tail of the string where useful identifiers (such as the number of date of the document) often live.

comment:13 Changed 10 years ago by benzea

Hm, I am not sure if there is an ideal way to fix this ...

I first thought that the best way is to limit the size of the palette in do_size_request. This is rather simple, but one runs into the issue, that the label requests a way too small area if ellipsizing is enabled. Wich means you the palettes are also too small.

So how about:

  1. Limit the size to a certain maximum in do_size_request (make this value quite large, but eg. make sure that it is not larger than the screen). Keep in mind that a eg. a colorpicker palette may be quite large.
  2. Set max_width_chars to an insanely large value, and enable ellipsizing.

The first point makes sure the palette does not get too large, the second one enforces the label to try to allocate as much space is as needed. (If you do not set max_width_chars, then it will only request enough space to show the '...'.)

The corner case are still menu items with the names in them (eg. in the home view when starting the journal entry). Their size is also larger than the screen, but they may not have the hack (ie. point 2) in place. This means they will not be ellipsized, but instead the string is just truncated at some point.

Changed 10 years ago by benzea

patch implementing the suggestion

comment:14 Changed 10 years ago by eben

The palette size shouldn't be limited by the palette class. This should be settable by the creator of the palette. For instance, palettes in the Journal, or in the home/groups/neighborhood views, should be a maximum of 1/2 the width of the screen, so that one edge of them can always align with the mouse cursor. On the other hand, palettes attached to toolbar buttons could be up to the full width of the screen. (This handles your color-picker case.)

Also, why shouldn't we just set max_width_chars to be a large value, with ellipsizing, in the palette class? The creator of the palette should indicate a maximum size for the palette, and the palette itself should ensure that the primary and secondary labels are treated accordingly, ellipsizing if needed.

comment:15 Changed 10 years ago by benzea

  • Cc benzea added

comment:16 Changed 10 years ago by benzea

Attached a patch.

Instead of having the text_maxlen parameter, we could just have a maxwidth parameter, which could just be 0.5 if the maximum width should be half the screen size. It should be quite simple to change the patch in that way.

Another improvement would be to change menuitem.py to do the same text_maxwidth hack with a large max_width_chars value, and ellipsizing enabled by default.

comment:17 Changed 10 years ago by tomeu

Simple patch now limiting MenuItems as well:

diff --git a/src/sugar/graphics/menuitem.py b/src/sugar/graphics/menuitem.py
index a357d78..7af8a6a 100644
--- a/src/sugar/graphics/menuitem.py
+++ b/src/sugar/graphics/menuitem.py
@@ -28,7 +28,7 @@ import gtk
 from sugar.graphics.icon import Icon
 
 class MenuItem(gtk.ImageMenuItem):
-    def __init__(self, text_label=None, icon_name=None, text_maxlen=0,
+    def __init__(self, text_label=None, icon_name=None, text_maxlen=60,
                  xo_color=None, file_name=None):
         gobject.GObject.__init__(self)
         self._accelerator = None
diff --git a/src/sugar/graphics/palette.py b/src/sugar/graphics/palette.py
index 919721b..1c668b6 100644
--- a/src/sugar/graphics/palette.py
+++ b/src/sugar/graphics/palette.py
@@ -145,7 +145,7 @@ class Palette(gtk.Window):
     # DEPRECATED: label is passed with the primary-text property, accel_path 
     # is set via the invoker property, and menu_after_content is not used
     def __init__(self, label=None, accel_path=None, menu_after_content=False,
-                 text_maxlen=0, **kwargs):
+                 text_maxlen=60, **kwargs):
 
         self.palette_state = self.PRIMARY
 

comment:18 Changed 10 years ago by tomeu

  • Keywords r? added

comment:19 Changed 10 years ago by tomeu

  • Milestone changed from 0.84 to 0.86
  • Severity changed from Blocker to Minor

r+'ed by Simon in #sugar

fixed by http://git.sugarlabs.org/projects/sugar-toolkit/repos/mainline/commits/b219dff0

Moving to 0.86 in case people want to fix it in a better way

comment:20 Changed 10 years ago by tomeu

  • Keywords r? removed

comment:21 Changed 10 years ago by tomeu

  • Milestone changed from 0.86 to 0.88

comment:22 Changed 9 years ago by garycmartin

  • Milestone changed from 0.88 to 0.90
  • Resolution set to fixed
  • Status changed from new to closed

Closing this ticket, if folks don't like this specific fix, a new ticket could be opened in the future.

comment:23 Changed 9 years ago by garycmartin

  • Milestone changed from 0.90 to 0.88

comment:24 Changed 6 years ago by dnarvaez

  • Milestone 0.88 deleted

Milestone 0.88 deleted

Note: See TracTickets for help on using tickets.