GNOME Bugzilla – Bug 753533
[year view] Don't force wide screens, don't limit to 3 rows x 4 columns
Last modified: 2017-04-17 18:20:40 UTC
The year view currently is somewhat fixed in its UI: 1. The view is limited to a fixed with of 4 months and a fixed height of 3 months (4 columns, 3 rows). This could be more flexible to allow 4x3, 2x6 and 6x2. This would allow to half-maximize gnome-calendar e.g. on FullHD screens (1920x1080 pixel). It would allow displaying gnome-calendar on screens less than 1048 pixel wide. And it could resolve this bug: https://bugzilla.gnome.org/show_bug.cgi?id=748884 2. When resizing the gnome-calendar window to make it as small as possible, you will see a huge margin/padding between the single months. This isn't necessary (and not present if window is bigger) but wastes space. You could get rid of that. 3. When the window is big enough, you could increase the font size instead of increasing margin/padding between single days.
Created attachment 325396 [details] [review] year-view: Multiple Columns Make year view more flexible being able to handle different number of columns. View was limited to only show months in a 4x3 grid which was causing issues when users were trying to use in different screens sizes. It was added code to implement a dynamic layout, so year view will adapt the number of months shown per row, depending on the view width.
Review of attachment 325396 [details] [review]: This patch made my day. Some fixes are needed, but this definitely is going very quick to the correct direction now. ::: data/ui/year-view.ui @@ +10,2 @@ <property name="visible">True</property> + <property name="shadow_type">out</property> The shadow type should be "none". @@ +12,3 @@ + <property name="vexpand">True</property> + <child> + <object class="GtkDrawingArea" id="navigator"> The direct child of the scrolled window must be a GtkViewport (which will contain the GtkDrawingArea). In this viewport, you should set the shadow type to "none" as well. ::: src/gcal-year-view.c @@ +28,3 @@ #define NAVIGATOR_CELL_WIDTH (210 + 15) #define NAVIGATOR_CELL_HEIGHT 210 +#define NAVIGATOR_HEADER_HEIGHT 52 I'm not really into adding more hardcoded sizes. Maybe if we calculate the header size based on the font? I'm OK with changing this in another patch following this one, but I don't want more hardcoded sizes for now. @@ +936,3 @@ + real_padding_top = (height - NAVIGATOR_CELL_HEIGHT * year_view->number_of_rows) / (year_view->number_of_rows + 1); + if (real_padding_top < 0) + real_padding_top = 0; Jump a line before the 'if' statement. @@ +1202,3 @@ + + gtk_widget_set_size_request (year_view->navigator, + (NAVIGATOR_CELL_WIDTH + padding_left) * year_view->number_of_columns, This should be calculated as (padding.start + WIDTH + padding.end). @@ +1203,3 @@ + gtk_widget_set_size_request (year_view->navigator, + (NAVIGATOR_CELL_WIDTH + padding_left) * year_view->number_of_columns, + (NAVIGATOR_CELL_HEIGHT + padding_top) * year_view->number_of_rows + NAVIGATOR_HEADER_HEIGHT); This should be calculated as (padding.top + HEIGHT + padding.bottom). @@ +1205,3 @@ + (NAVIGATOR_CELL_HEIGHT + padding_top) * year_view->number_of_rows + NAVIGATOR_HEADER_HEIGHT); + + year_view->popover_mode = (alloc->width < (NAVIGATOR_CELL_WIDTH + padding_left) * year_view->number_of_columns + SIDEBAR_PREFERRED_WIDTH); The popover mode definitely is not OK anymore. It's behavior is nearly unpredictable now. We should review its behavior. I'd go with only showing the popover when the width < 720, or height < 600.
Created attachment 325416 [details] [review] year-view: Multiple Columns Make year view more flexible being able to handle different number of columns. View was limited to only show months in a 4x3 grid which was causing issues when users were trying to use in different screens sizes. It was added code to implement a dynamic layout, so year view will adapt the number of months shown per row, depending on the view width.
(In reply to Georges Basile Stavracas Neto from comment #2) > The shadow type should be "none". DONE > The direct child of the scrolled window must be a GtkViewport (which will > contain the GtkDrawingArea). DONE > In this viewport, you should set the shadow type to "none" as well. DONE > I'm not really into adding more hardcoded sizes. Maybe if we calculate the > header size based on the font? That's my idea for a future patch. Not only that but other hardcoded sizes as well. > Jump a line before the 'if' statement. DONE > This should be calculated as (padding.start + WIDTH + padding.end). DONE > This should be calculated as (padding.top + HEIGHT + padding.bottom). DONE > The popover mode definitely is not OK anymore. It's behavior is nearly > unpredictable now. > > We should review its behavior. I'd go with only showing the popover when the > width < 720, or height < 600. About that, I've recorded a video with the current behavior, so you can check if that is OK or not: https://youtu.be/J3UKb9NgDCg IMHO, it's fine, but we can change it in a future patch as well.
Review of attachment 325416 [details] [review]: Only some nitpicks now. ::: data/ui/year-view.ui @@ +7,3 @@ </style> <child> + <object class="GtkScrolledWindow" id="navigator_scroll"> Since we're not using the scrolled window anywhere in the code, it doesn't need an 'id' set. @@ +12,3 @@ + <property name="vexpand">True</property> + <child> + <object class="GtkViewport" id="navigator_viewport"> Ditto. ::: src/gcal-year-view.c @@ +1177,2 @@ if (minimum != NULL) + *minimum = NAVIGATOR_CELL_WIDTH + padding_left * 2; This code should also use 'padding_left + padding_right' @@ +1179,2 @@ if (natural != NULL) + *natural = NAVIGATOR_CELL_WIDTH + padding_left * 2 + SIDEBAR_PREFERRED_WIDTH; Ditto. @@ +1207,3 @@ + padding_top + NAVIGATOR_CELL_HEIGHT * year_view->number_of_rows + padding_bottom + NAVIGATOR_HEADER_HEIGHT); + + year_view->popover_mode = (alloc->width < padding_left + NAVIGATOR_CELL_WIDTH * year_view->number_of_columns + padding_right + SIDEBAR_PREFERRED_WIDTH); Simplify the calcs by using 'hpadding = left + right' and 'vpadding = top + bottom'
Created attachment 325440 [details] [review] year-view: Multiple Columns Make year view more flexible being able to handle different number of columns. View was limited to only show months in a 4x3 grid which was causing issues when users were trying to use in different screens sizes. It was added code to implement a dynamic layout, so year view will adapt the number of months shown per row, depending on the view width.
Review of attachment 325440 [details] [review]: LGTM. I'll push this patch together with the next one (fixing hardcoded sizes).
Created attachment 325761 [details] [review] year-view: Multiple Columns (part 2) Improve year-view code, removing hardcoded values. year-view have hardcoded values for grid cell sizes (width and height). Those values need to be recalculated every time a change in font or any other style are changed. This patch changes that behavior by calculating those values using CSS style values for font size and padding. This patch also complements commit https://bugzilla.gnome.org/attachment.cgi?id=325440&action=diff adding code to resize and redraw the window when show-week-numbers property is changed. After the scroll bar was added to navigator, the view was not updating the content as it should.
Review of attachment 325761 [details] [review]: About the commit message: it'd be better if you write something in the lines of "removed hardcoded sizes" instead of "part 2". ::: src/gcal-year-view.c @@ +29,2 @@ #define SIDEBAR_PREFERRED_WIDTH 200 +#define SIDEBAR_MIN_WIDTH 800 Too much. Also, isn't the purpose of this patch to remove all hardcoded sizes? @@ +30,3 @@ +#define SIDEBAR_MIN_WIDTH 800 +/* Gtk+ doesn't count window decoration, so... */ +#define SIDEBAR_MIN_HEIGHT 555 Ditto. @@ +1292,3 @@ vpadding = padding_top + padding_bottom; + + year_view->number_of_columns = (alloc->width - (SIDEBAR_PREFERRED_WIDTH * !year_view->popover_mode)) / (year_view->column_width + hpadding); It'd be better if we use a constraint to decide the sidebar width. I'd go for 1/4 of the window's width.
Created attachment 326516 [details] [review] year-view: Remove hardcoded values Improve year-view code, removing hardcoded values. year-view have hardcoded values for grid cell sizes (width and height). Those values need to be recalculated every time a change in font or any other style are changed. This patch changes that behavior by calculating those values using CSS style values for font size and padding. This patch also complements commit https://bugzilla.gnome.org/attachment.cgi?id=325440&action=diff adding code to resize and redraw the window when show-week-numbers property is changed. After the scroll bar was added to navigator, the view was not updating the content as it should. Popover mode was also changed and now sidebar will always be shown when show-week-numbers is set, but if it isn't set, a popover will be shown when there is space for only 3 or less columns.
(In reply to Georges Basile Stavracas Neto from comment #9) > About the commit message: it'd be better if you write something in the lines > of "removed hardcoded sizes" instead of "part 2". I've done a mistake and instead of a commit --amend, I did a simple commit, so please consider applying both patches or if you like, tell me how to merge then. > Too much. Also, isn't the purpose of this patch to remove all hardcoded > sizes? DONE > It'd be better if we use a constraint to decide the sidebar width. I'd go > for 1/4 of the window's width. Sidebar size is defined in the .ui file and I've kept that. One possible solution is to set sidebar width to the same width of the month columns.
Created attachment 326562 [details] [review] year-view: Remove sidebar hardcoded width Improve year-view code, removing hardcoded values. Sidebar width was hardcoded instead of getting directly from the ui file. That would prevent if ui was changed, the navigator code wouldn't notice it and would draw its content wrongly. This patch get sidebar width from the ui file.
(In reply to igaldino from comment #11) > (In reply to Georges Basile Stavracas Neto from comment #9) > > It'd be better if we use a constraint to decide the sidebar width. I'd go > > for 1/4 of the window's width. > Sidebar size is defined in the .ui file and I've kept that. One possible > solution is to set sidebar width to the same width of the month columns. The last (3rd :)) patch only removes the #define directive and gets the sidebar width from the .ui file, which today is 200 pixels. I need your go ahead about keeping it that way or changing it according to the width of the month columns (I've tested it already and it works ;)).
Could you please squash the last 3 patches?
Created attachment 326814 [details] [review] year-view: Remove hardcoded values Improve year-view code, removing hardcoded values. year-view have hardcoded values for grid cell sizes (width and height). Those values need to be recalculated every time a change in font or any other style are changed. Sidebar width was also hardcoded instead of getting its value directly from the ui file. That would prevent it from drawing the content accordingly when ui file is changed. This patch fixes those two issues by retrieving those values from CSS or ui files. This patch also adds code to resize and redraw the window when show-week-numbers property is changed. Popover mode was also changed and now sidebar will always be shown when show-week-numbers is set or when there is space for only 3 or less columns.
(In reply to Georges Basile Stavracas Neto from comment #14) > Could you please squash the last 3 patches? DONE Please remove the old ones. Thanks.
Review of attachment 326814 [details] [review]: The code is starting to take shape. A small issue I found is that there is no way to scroll while selecting dates in the date navigator. ::: src/gcal-year-view.c @@ +71,3 @@ + guint row_height; + guint header_height; + guint sidebar_width; How many of these fields can be calculated on the fly? Adding too many fields here leads to many pointer lookups, which may have a minimal but important impact on the performance of Year view. @@ +1131,3 @@ + + /* measure header height */ + test_str = g_strdup_printf ("8888"); Using this fixed string is a bad idea. Some font families may have 9 or 6 as the tallest numbers. Since the header height is already calculated in another section of Year view (see draw_navigator() function), it'd be better if you factor that code out in a function and reuse this function here. @@ +1155,3 @@ + + /* measure box size */ + test_str = g_strdup_printf ("88"); Ditto.
I pushed the current work on master, so other ones may test it. The current issues I see: - With my 1366x768 screen, it has only 2 rows and 6 columns. Maybe we could add more padding between the columns so it falls back to 3 rows and 4 columns. - It is impossible to scroll while selecting. Otherwise, the work is really great. A big thanks for working on that, Isaque. Attachment 326814 [details] pushed as 97d8859 - year-view: Remove hardcoded values
Created attachment 328015 [details] [review] year-view: Reduce maximum of month columns The view had a maximum of 6 month columns, leading to a huge amount of vertical spaces, so it was requested to reduce that. The maximum number of columns was reduced to 4, so the view will have a maximum 4x3 months layout.
I'm not sure fixed constraints are necessarily the right solution, we don't know what sort of weird screen the user is working on or what font size he is setting. I think between month spacing should be a function of the size of the months itself. As an example, a month (in the year view) is like a box, this box's width and height should not be exceeded (or maybe only even half or 3/4th of it) in the space between 2 month columns or rows. In case there's 'infinite' (enough) width and height to show a calendar in, then we could fall back to a default view of 3x4. Likewise, there should be minimum constraints (1/4th?) etc. I think hard-coding this is gonna come back to us at some point.
(In reply to Marinus Schraal from comment #20) > I'm not sure fixed constraints are necessarily the right solution, we don't > know what sort of weird screen the user is working on or what font size he We'll need to test it to make sure. We're trying to balance a layout that works and looks good. My screen is 1366x768 only, so we need help to see how it fits other screen sizes. > As an example, a month (in the year view) is like a box, this box's width > and height should not be exceeded (or maybe only even half or 3/4th of it) Even here we would have a fixed constraint (1/2, 3/4, 1/4,...). > in the space between 2 month columns or rows. In case there's 'infinite' > (enough) width and height to show a calendar in, then we could fall back to > a default view of 3x4. Likewise, there should be minimum constraints That's what it does now, if there is enough width, it will be 4x3 and the remaining space will be placed among the columns.
(In reply to Isaque Galdino from comment #21) > (In reply to Marinus Schraal from comment #20) > > I'm not sure fixed constraints are necessarily the right solution, we don't > > know what sort of weird screen the user is working on or what font size he > We'll need to test it to make sure. We're trying to balance a layout that > works and looks good. My screen is 1366x768 only, so we need help to see how > it fits other screen sizes. What I'm saying is that design wise fixed layouts are a short-cut. A 3x4 layout is fine, but in some circumstances it might not be. A flowing lay-out does the visually correct thing automatically. > > As an example, a month (in the year view) is like a box, this box's width > > and height should not be exceeded (or maybe only even half or 3/4th of it) > Even here we would have a fixed constraint (1/2, 3/4, 1/4,...). It's not fixed in the sense that it's a relative measure to the size of a full calendar box. Spacing is a relative measure of the objects to be spaced.
I was thinking here, and I came up with a good heuristic to decide how many columns it'll have on resizing. - There will be a score(int cols) method that will calculate how much unused space there will be for the given number of columns. - It iterates from 1 to 12 and calculates the score(i) for each column. - The column with the highest score is the selected one. - Proceed and draw the year navigator with the selected number of columns. What do you guys think?
Per Allan's comment, I'm going ahead and applying the 4-column-limit patch. With that, I believe the purpose of this bug is fullfilled and we can finally have a calm rest. Thanks for all the blazingly hard work, Isaque. Attachment 328015 [details] pushed as f32fd7d - year-view: Reduce maximum of month columns
Looks pretty good now, thank you very much!