GNOME Bugzilla – Bug 763217
[month-view] draw just the outline on multi-day selection
Last modified: 2017-04-17 18:20:40 UTC
Currently the multi-day selection draws a number of boxes if more than one week is involved in the selection. Add a new draw mode for the irregular shaped box that just draws the outline.
Created attachment 323256 [details] [review] month-view: beautify multi-day selection outline Draw just the outline for the irregular shaped outline of multi-day selection.
Created attachment 323257 [details] new selection the new outline
Created attachment 323258 [details] selection outline thick An example with a thick outline (since the other is very hard to actually see).
Review of attachment 323256 [details] [review]: Code-wise, the patch looks good. Please, improve the commit message (follow the guidelines at [1]). Also, I wonder if we could draw a slightly blued background on selected cells (if you manage to make that, please share a screenshot). [1] https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines ::: src/gcal-month-view.c @@ +1694,3 @@ + gint last_row = last_mark / 7; + gint first_column = first_mark % 7; + gint last_column = last_mark % 7; Initialize the variables after declaring them (starts at "start_x = ..."). @@ +1697,3 @@ + + gint start_x, start_y, end_x, end_y; + gint max_x = cell_width * 7; Ditto @@ +1708,3 @@ + cairo_move_to (cr, end_x + 10, end_y); + cairo_arc (cr, end_x, end_y, 10, 0, 2 * M_PI); + */ Remove this debug code. @@ +1710,3 @@ + */ + + cairo_move_to (cr, start_x + 0.3, start_y + 0.3); Add a detailed comment explaining the logic of the line_to() calls.
Created attachment 323427 [details] [review] month-view: fix line width inconsistencies The line width used for the grid changed over time, resulting in different values used at different places. Introduce a line_widht variable and use it consistently throughout when drawing.
Created attachment 323428 [details] [review] month-view: cleanup multi-row selection drawing The selection mode was drawing a number of boxes on multi-row selections, resulting in unwanted outlines within the selection area. This patch adds a multirow mode that just draws the outline of a selection. Make cell_{width,height} integers so divison lines always get drawn on exact grid coordinates, otherwise line calculations could result in fuzzy lines.
Created attachment 323429 [details] [review] month-view: add selection background fill
Review of attachment 323427 [details] [review]: A small nitpick. Besides that, looks good. ::: src/gcal-month-view.c @@ +1355,3 @@ gint i, j, sw, lower_mark = 43, upper_mark = -2; gdouble start_grid_y, cell_width, cell_height, first_row_gap = 0.0; + gdouble line_width = 0.5; Put this in a #define.
Review of attachment 323428 [details] [review]: Code looks ok, needs only some style changes. ::: src/gcal-month-view.c @@ -1646,3 @@ cairo_set_line_width (cr, line_width); - /* horizontals */ Why did you remove this comment? @@ +1649,3 @@ if ((first_mark / 7) == (last_mark / 7)) { + /* selection all on the same row */ Didn't understand the comment. @@ +1714,3 @@ + cairo_move_to (cr, start_x, start_y); + cairo_line_to (cr, max_x, start_y); + if (last_column == 6) Jump a line before the 'if' and add a comment explaining the difference between drawing in the last column. @@ +1724,3 @@ + cairo_line_to (cr, end_x, end_y); + } + cairo_line_to (cr, (line_width / 2), end_y); Jump a line between '}' and 'cairo_line_to(...)'.
Review of attachment 323429 [details] [review]: LGTM.
Created attachment 323753 [details] [review] month-view: fix line width inconsistencies The line width used for the grid changed over time, resulting in different values used at different places. Introduce a LINE_WIDTH definition and use it consistently throughout when drawing.
Created attachment 323754 [details] [review] month-view: cleanup multi-row selection drawing The selection mode was drawing a number of boxes on multi-row selections, resulting in unwanted outlines within the selection area. This patch adds a multirow mode that just draws the outline of a selection. Make cell_{width,height} integers so divison lines always get drawn on exact grid coordinates, otherwise line calculations could result in fuzzy lines.
(In reply to Georges Basile Stavracas Neto from comment #9) > Review of attachment 323428 [details] [review] [review]: > - /* horizontals */ > > Why did you remove this comment? Horizontals is not really what it is anymore and the comments have been moved into the 'if (else)' parts. > > @@ +1649,3 @@ > if ((first_mark / 7) == (last_mark / 7)) > { > + /* selection all on the same row */ > > Didn't understand the comment. clarified
Created attachment 323758 [details] [review] month-view: cleanup multi-row selection drawing The selection mode was drawing a number of boxes on multi-row selections, resulting in unwanted outlines within the selection area. This patch adds a multirow mode that just draws the outline of a selection. Make cell_{width,height} integers so divison lines always get drawn on exact grid coordinates, otherwise line calculations could result in fuzzy lines.
Thanks for your work on this issue. Attachment 323429 [details] pushed as 8a72bf0 - month-view: add selection background fill Attachment 323753 [details] pushed as 0fce5ba - month-view: fix line width inconsistencies Attachment 323758 [details] pushed as 0ce85f6 - month-view: cleanup multi-row selection drawing