GNOME Bugzilla – Bug 614144
placesDisplay: Prevent bookmarks from being cut off
Last modified: 2010-03-31 15:09:19 UTC
The port of placesDisplay to St introduced a regression that can cause the bookmarks being cut off when the right column has more items than the left one.
Created attachment 157296 [details] [review] placesDisplay: Prevent bookmarks from being cut off Moving to St.Table introduced a regression that resulted in the height of Places section to only depend on the height of the left column. This could result into some bookmarks not being displayed at all because there are not enough items in the left column to allocate the needed height. Fix this by using a nested table for the left column rather than rowspan for the right column.
*** Bug 614143 has been marked as a duplicate of this bug. ***
Created attachment 157313 [details] [review] placesDisplay: Prevent bookmarks from being cut off Moving to St.Table introduced a regression that resulted in the height of Places section to only depend on the height of the left column. This could result into some bookmarks not being displayed at all because there are not enough items in the left column to allocate the needed height. Fix this by using a nested table for the left column rather than rowspan for the right column.
Review of attachment 157313 [details] [review]: ::: js/ui/placeDisplay.js @@ -517,3 @@ // but it seems like we want some sort of differentiation between actions - homogeneous: true }); - this.actor = new St.Table({ style_class: 'places-section', The homogeneous property is set to enforce equal width to the columns. @@ +529,3 @@ // Subdivide left into actions and devices + this._leftCol = new St.Table({ homogeneous: true }); A one-column table should probably just be a BoxLayout (see the previous version of the breaking commit), although a much nicer fix would be to either make it work using some combination of expand/fill properties on the table children or (if that doesn't work out) to investigate how StTable could be fixed to deal with situations like that (see Dan's comment on the original patch in bug 610385)
(In reply to comment #4) > Review of attachment 157313 [details] [review]: > > ::: js/ui/placeDisplay.js > @@ -517,3 @@ > // but it seems like we want some sort of differentiation between > actions > - homogeneous: true }); > - this.actor = new St.Table({ style_class: 'places-section', > > The homogeneous property is set to enforce equal width to the columns. The previous patch did that (the one I obsoleted with this one), the reason for this was that it looks bad this way (the right column was to far on the left). > @@ +529,3 @@ > // Subdivide left into actions and devices > > + this._leftCol = new St.Table({ homogeneous: true }); > > A one-column table should probably just be a BoxLayout (see the previous > version of the breaking commit), OK, true using a table for one column is a bit pointless. > although a much nicer fix would be to either > make it work using some combination of expand/fill properties on the table > children or (if that doesn't work out) I tried this it does not seem to have any effect. > to investigate how StTable could be > fixed to deal with situations like that (see Dan's comment on the original > patch in bug 610385) The problem is how row_span is handled. it just allocates the height of first row and multiply it by the rowspan. Which in this case means the height of the right column is the height of the first row in the left column (0, 0) * 2. Which is not guaranteed to include all bookmarks hence the regression.
Created attachment 157321 [details] [review] placesDisplay: Prevent bookmarks from being cut off Moving to St.Table introduced a regression that resulted in the height of Places section to only depend on the height of the left column. This could result into some bookmarks not being displayed at all because there are not enough items in the left column to allocate the needed height. Fix this by using a St.BoxLayout for the left column rather than rowspan for the right column.
When I suggested moving to StTable in 610385, I didn't realize that _leftBox and _rightBox had additional boxes as children rather than DashPlaceDisplayItems directly. So what I was envisioning was more like having _leftBox, _rightBox, _actionsBox, _devBox, and _dirsBox *all* go away. If that is too annoying to do because of bookkeeping (eg, being able to do _updateMounts without affecting the bookmarks) then yes, there's no reason to have a table at the top level.
(In reply to comment #7) > there's no reason to have a table at the top level. The reason I used a table as top level was to have both columns of equal width, which I couldn't get to work using a BoxLayout. Dropping _all_ the boxes sounds doable though.
*** Bug 614286 has been marked as a duplicate of this bug. ***
Created attachment 157454 [details] [review] placesDisplay: Prevent bookmarks from being cut off Moving to St.Table introduced a regression that resulted in the height of Places section to only depend on the height of the left column. This could result into some bookmarks not being displayed at all because there are not enough items in the left column to allocate the needed height. Fix this by removing the St.BoxLayout actors and add the items directly to the table.
Created attachment 157456 [details] [review] placesDisplay: Prevent bookmarks from being cut off
Created attachment 157468 [details] [review] placesDisplay: Prevent bookmarks from being cut off *) Fix spacing
Created attachment 157489 [details] [review] placesDisplay: Prevent bookmarks from being cut off Fix resize issue when removing bookmarks.
Created attachment 157490 [details] [review] StTable: Add st_table_resize Add a method that resizes the table to a given size, similar to gtk_table_resize.
Comment on attachment 157490 [details] [review] StTable: Add st_table_resize >+ * st_table_get_column_count: wrong name :) >+ * @cols: Number of columns >+ * Resize the the table @table to a the given size need a blank line between the last parameter and the description. "to a the" should be "to the" And should explain when/why this is needed. "Resizes @table to the given size, which must be at least large enough to fit all of the table's current children. This can be used to shrink the row or column count after removing children." >+ * Returns: %TRUE on success, otherwise %FALSE is that really useful? > gint st_table_get_row_count (StTable *table); > gint st_table_get_column_count (StTable *table); >+gboolean st_table_resize (StTable *table, gint rows, gint cols); add spaces after "gint" on the first two lines to align them with the new function, and add newlines after the commas (and align the parameters). the actual code looks fine.
(In reply to comment #15) > (From update of attachment 157490 [details] [review]) > >+ * st_table_get_column_count: > > wrong name :) Damn ... copy/paste ;) > >+ * @cols: Number of columns > >+ * Resize the the table @table to a the given size > > need a blank line between the last parameter and the description. OK > "to a the" should be "to the" err... OK > And should explain when/why this is needed. "Resizes @table to the given size, > which must be at least large enough to fit all of the table's current children. > This can be used to shrink the row or column count after removing children." OK > >+ * Returns: %TRUE on success, otherwise %FALSE > > is that really useful? No idea, but it might be useful in some cases to know whether it actually did anything. > > gint st_table_get_row_count (StTable *table); > > gint st_table_get_column_count (StTable *table); > >+gboolean st_table_resize (StTable *table, gint rows, gint cols); > > add spaces after "gint" on the first two lines to align them with the new > function, and add newlines after the commas (and align the parameters). OK > the actual code looks fine. OK
Created attachment 157498 [details] [review] StTable: Add st_table_resize Add a method that resizes the table to a given size, similar to gtk_table_resize.
Comment on attachment 157498 [details] [review] StTable: Add st_table_resize >+ * This can be used to shrink the row or column count after removing children. >+ * >+ */ there's some inconsistency about this in st, but generally there shouldn't be a blank line at the end of a gtk-doc comment >+ g_return_if_fail (ST_IS_TABLE (table)); >+ StTablePrivate *priv = ST_TABLE (table)->priv; oops, missed this before. We're sticking to C89 syntax, so you can't have declarations after code, so you have to have the declaration here, then the g_return_if_fail() after the declarations, then "priv = table->priv" after that. (You don't need the "ST_TABLE()" because table is already declared as an StTable.) >+void st_table_resize (StTable *table, >+ gint rows, >+ gint cols); add spaces after the "gint"s to align "rows" and "cols" with "table". (The "r" and "c" under the "t", not under the "*".) OK to commit with these fixes.
(In reply to comment #15) > And should explain when/why this is needed. "Resizes @table to the given size, > which must be at least large enough to fit all of the table's current children. > This can be used to shrink the row or column count after removing children." Can we make StTable just do this automatically? Seems annoying to require an app to explicit curate the number of rows and columns to avoid extra spacing.
(In reply to comment #19) > (In reply to comment #15) > > And should explain when/why this is needed. "Resizes @table to the given size, > > which must be at least large enough to fit all of the table's current children. > > This can be used to shrink the row or column count after removing children." > > Can we make StTable just do this automatically? Seems annoying to require an > app to explicit curate the number of rows and columns to avoid extra spacing. Well yes we could basically do this in st_table_remove_actor.
Created attachment 157506 [details] [review] StTable: Update row and column count in st_table_remove_actor Adjust the table size when removing an actor in st_table_remove_actor.
Created attachment 157507 [details] [review] placesDisplay: Prevent bookmarks from being cut off Make use of new StTable changes.
Created attachment 157508 [details] [review] placesDisplay: Prevent bookmarks from being cut off *) Fix style issue
(In reply to comment #19) > Can we make StTable just do this automatically? Seems annoying to require an > app to explicit curate the number of rows and columns to avoid extra spacing. GtkTable doesn't... I suppose the idea is that this makes removing an actor O(n) since you have to scan the child list, and so if the table is really big then... Except that removing the actor from priv->children is already O(n), so... yeah, we should just do it.
Comment on attachment 157506 [details] [review] StTable: Update row and column count in st_table_remove_actor I would remove the child first, then update the counts. (And then you don't need the "if child == actor" check.) also, you need to g_object_notify (G_OBJECT (table), "row-count")); if (and only if) priv->n_rows changes, and likewise with col-count. (And likewise in st_table_resize(), if you're still keeping that, which it's not clear we need to do.) Yes, the existing code is mostly wrong about this. We need to go through St and add a whole bunch of missing g_object_notifies at some point.
(In reply to comment #25) > (From update of attachment 157506 [details] [review]) > I would remove the child first, then update the counts. (And then you don't > need the "if > child == actor" check.) I did this on purpose, the idea was that otherwise I'd have to queue a relayout at the end of the function (otherwise the change wouldn't have any visual effect), but that way _st_container_remove_actor should already do this so we don't have to do it twice. > also, you need to > > g_object_notify (G_OBJECT (table), "row-count")); > > if (and only if) priv->n_rows changes, and likewise with col-count. (And > likewise in st_table_resize(), if you're still keeping that, which it's not > clear we need to do.) Yes, the existing code is mostly wrong about this. We > need to go through St and add a whole bunch of missing g_object_notifies at > some point. OK
Created attachment 157512 [details] [review] StTable: Update row and column count in st_table_remove_actor Adjust the table size when removing an actor in st_table_remove_actor.
Comment on attachment 157512 [details] [review] StTable: Update row and column count in st_table_remove_actor >+ for (list = priv->children; list; list = list->next) >+ { >+ ClutterActor *child = CLUTTER_ACTOR (list->data); the "{" and "}" should be indented 2 spaces from the "for", and the body indented another 2 spaces from there. >+ >+ if (priv->n_rows != n_rows) put a "g_object_freeze_notify (G_OBJECT (container));" before this >+ g_object_notify (G_OBJECT (container), "column-count"); >+ } and a thaw_notify after this. good to commit with those fixes
Comment on attachment 157508 [details] [review] placesDisplay: Prevent bookmarks from being cut off >+ this._actionList = new Array(); >+ this._dirList = new Array(); >+ this._deviceList = new Array(); just say "[]", not "new Array()" (likewise in _updateDefaults, etc) Also, while you're redoing this, can we switch to only having one name for each group? (Eg, change actionList to defaultsList to match updateDefaults, and likewise dirList to bookmarksList and deviceList to mountsList.) >- this._actionsBox.destroy_children(); >+ for (let i = 0; i < this._actionList.length; i++) >+ this.actor.remove_actor(this._actionList[i]); It would be more consistent with the old code to do "this._actionList[i].destroy()". Destroying the actor will automatically cause it to be removed from its parent, and will also make sure we immediately free up any X/GL/etc resources that the actor was using (which otherwise won't get released until the actor is garbage-collected by gjs). _updateDefaults() should call _updateMounts() at the end, since if the number of defaults changed, then the mounts need to be repositioned higher or lower as well.
Created attachment 157592 [details] [review] placesDisplay: Prevent bookmarks from being cut off Moving to St.Table introduced a regression that resulted in the height of Places section to only depend on the height of the left column. This could result into some bookmarks not being displayed at all because there are not enough items in the left column to allocate the needed height. Fix this by removing the St.BoxLayout actors and add the items directly to the table.
Attachment 157592 [details] pushed as 7506720 - placesDisplay: Prevent bookmarks from being cut off