GNOME Bugzilla – Bug 771581
(Month view) 'other %d events' and date labels get mixed up
Last modified: 2017-05-24 21:05:47 UTC
Created attachment 335755 [details] Small Calendar window with mixed up labels (image) The button 'other %d labels' and date labels get mixed up when the calendar window is too small. Please see the attachment. See the date Sept 21 and 30.
Created attachment 350257 [details] [review] Fix 'other %d' mixed-up with date label Fix 'other %d' mixed-up with date label
Since "Other %d events" label uses whole cell width as width, we can substract date label's width from this width so as to create some space for the date label.
Review of attachment 350257 [details] [review]: Thanks for the patch! I have a few comments. First, looks like your author name is broken in git. Here, the only thing I can see is " <°6¦"V>". If possible, please stick to Unicode / UTF-8. The commit message needs to be rewritten (or in this case, written). The GNOME Newcomers Guide has an excellent example of how commit messages should be written, please make sure to check it out here: https://wiki.gnome.org/Newcomers/SubmitPatch At last, Calendar uses the GNU C Coding Style, please make sure to stick to it in future patches. The code review is below. ::: src/views/gcal-month-view.c @@ +1655,2 @@ pango_layout_set_font_description (overflow_layout, ofont_desc); + pango_layout_set_width (overflow_layout, pango_units_from_double (cell_width - strlen (nr_day)*font_width)); Please use g_utf8_strlen(), and put spaces around '*'. This code is wrong. The 'font_width' variable does not hold anything related to the month day width. This is calculated below, and this whole 'if' block should be moved below it.
Created attachment 350386 [details] [review] Fix "Other X events" label mixed up with date label The overflow layout used to display "Other X events" text when there is no enough space to accomodate more events. This used to get mixed-up/overlapped with the date label because the overflow label uses whole cell width as width. We can fix this by substracting date label width from overflow label width to accomodate the date label.
(In reply to Abdullahi Usman from comment #4) > Created attachment 350386 [details] [review] [review] > Fix "Other X events" label mixed up with date label > > > The overflow layout used to display "Other X events" text when > there is no enough space to accomodate more events. > > This used to get mixed-up/overlapped with the date label because the > overflow label uses whole cell width as width. > > We can fix this by substracting date label width from overflow label > width to accomodate the date label. Okay
Review of attachment 350386 [details] [review]: The patch is working well, thanks! Two comments: - This patch exposed an issue where shrinking down Calendar's size makes the "Other X events" label use 2 lines. Could you please fix that in your patch as well? The label should ellipsize at the end. - On a side note, I noticed that the "Other X events" label doesn't respect the CSS margin property. Could you please craft a new patch to adress that?
Created attachment 351146 [details] [review] adjust overflow layout and set it ellipsized at end
Created attachment 351147 [details] [review] add margin to overflow layout This patch addresses the issue is that the overflow layout does not have margin property like other views(layouts) in the cell, so instead it appears either too far from or too close to the border depending on the text it accomodates.
Review of attachment 351146 [details] [review]: One nitpick below. About the commit message, please remove the "src/views/gcal-" prefix, the '.c' and the space before ':'. ::: src/views/gcal-month-view.c @@ +1724,3 @@ + pango_layout_set_width (overflow_layout, pango_units_from_double (cell_width - (font_width + padding.right))); + pango_layout_set_alignment (overflow_layout, PANGO_ALIGN_CENTER); + gtk_style_context_save (context); Wrong style. Needs a space before '('. Please, always use the GNU C Coding Style on Calendar.
Review of attachment 351147 [details] [review]: One comment. The same observations about the commit message apply here. ::: src/views/gcal-month-view.c @@ -1723,3 @@ pango_layout_set_font_description (overflow_layout, ofont_desc); pango_layout_set_width (overflow_layout, pango_units_from_double (cell_width - (font_width + padding.right))); - pango_layout_set_alignment (overflow_layout, PANGO_ALIGN_CENTER); Why did you remove the center alignment?
Created attachment 351676 [details] [review] adjust overflow layout and set it ellipsized at end
Created attachment 351677 [details] [review] add margin to overflow layout
Created attachment 351695 [details] [review] add margin to overflow layout
Review of attachment 351676 [details] [review]: After testing this patch, I still see the problem described of the bug. I couldn't find any place where it's actually setting the font width in the code. ::: src/views/gcal-month-view.c @@ +1724,3 @@ + pango_layout_set_width (overflow_layout, pango_units_from_double (cell_width - (font_width + padding.right))); + pango_layout_set_alignment (overflow_layout, PANGO_ALIGN_CENTER); + pango_layout_set_ellipsize(overflow_layout, PANGO_ELLIPSIZE_END); Please, ~please~, fix the style issue here. It's missing a space before '('. Pretty please, follow the GNU C Coding Style when coding on Calendar.
Review of attachment 351695 [details] [review]: LGTM
(In reply to Georges Basile Stavracas Neto from comment #14) > Review of attachment 351676 [details] [review] [review]: > > After testing this patch, I still see the problem described of the bug. I > couldn't find any place where it's actually setting the font width in the > code. > > ::: src/views/gcal-month-view.c > @@ +1724,3 @@ > + pango_layout_set_width (overflow_layout, pango_units_from_double > (cell_width - (font_width + padding.right))); > + pango_layout_set_alignment (overflow_layout, PANGO_ALIGN_CENTER); > + pango_layout_set_ellipsize(overflow_layout, PANGO_ELLIPSIZE_END); > > Please, ~please~, fix the style issue here. It's missing a space before '('. > Pretty please, follow the GNU C Coding Style when coding on Calendar. Sorry about the coding style, won't happen again.
Created attachment 352184 [details] [review] adjust overflow layout and set it ellipsized at end
Created attachment 352187 [details] [review] [V2] add margin to overflow layout This is the same patch as the old one but this patch depends on "adjust overflow layout and set it elipsized at end" patch which was re-made, so thatz why this patch was also re-made.
Review of attachment 352184 [details] [review]: This patch is looking ~very~ good! Great job, thanks!
Review of attachment 352187 [details] [review]: This will need some more work. To correctly use the margins, you have to: 1. pango_layout_set_width (layout, w - margin.left - margin.right) 2. gtk_render_layout (..., x + margin.left) This patch only does #2. It also needs #1.
Comment on attachment 352184 [details] [review] adjust overflow layout and set it ellipsized at end Attachment 352184 [details] pushed as ad3e376 - month-view : adjust overflow layout and set it ellipsized at end
Created attachment 352479 [details] [review] [V3] add margin to overflow layout This is the same as [V2] except apart from the right margin, it now also set the left margin when setting the width of the layout.
Review of attachment 352479 [details] [review]: Looks very good, thanks!
Thanks for the patches, and congratulations for your first contributions in GNOME Calendar! :-) Attachment 352479 [details] pushed as 9bbb126 - month-view: add margin to overflow layout
(In reply to Georges Basile Stavracas Neto from comment #31) > Thanks for the patches, and congratulations for your first contributions in > GNOME Calendar! :-) > > Attachment 352479 [details] pushed as 9bbb126 - month-view: add margin to > overflow layout wow..., i've made it. Thanks you also. Lets go hunt for some bugs!