GNOME Bugzilla – Bug 742406
World Clock Redesign
Last modified: 2021-06-01 22:44:49 UTC
This bug shall hold discussions about the redesign of the World Clock component. The requirements for this component are currently developed here: https://wiki.gnome.org/action/edit/Apps/Clocks/WorldClockRedesign
A big question in the world clock redesign is for me if we want the time conversion feature. I can't imagine a real usecase right now.
(In reply to comment #1) > A big question in the world clock redesign is for me if we want the time > conversion feature. I can't imagine a real usecase right now. Arranging any kind of event that involves multiple time zones - calls, video chats, etc. There are plenty of online tools that exist for this reason.
(In reply to comment #2) > (In reply to comment #1) > > A big question in the world clock redesign is for me if we want the time > > conversion feature. I can't imagine a real usecase right now. > > Arranging any kind of event that involves multiple time zones - calls, video > chats, etc. There are plenty of online tools that exist for this reason. At least for me thats a rare occasion but for an application that names itself proudly clocks I assume we'd want to have that feature too. I added it and I'll leave it there if no one objects. Another question I want to raise is about analog/digital time representation. The clocks app on my phone allows me to switch between analog/digital representation. I'm not sure if we want that. Would be better if we could choose the 'best' representation - maybe a design that holds both representations in it would be suitable for most people, whatever they prefer?
(In reply to comment #1) > A big question in the world clock redesign is for me if we want the time > conversion feature. I can't imagine a real usecase right now. For me this is a must have, it is actually one of the things I miss the most in the current design. With regard to the wiki page, I'd add this to the goals: "the disposition of world clocks should reflect the geographic disposition" in other words I mean that if I am in Europe I expect US times to be on the left and Japan times to be on the right. This is one of the pain points of the current design. The proposed mockup addresses also this problem. In general I really like the proposed mockup. Here are some additional considerations that could be taken into account when iterating the design 1) I know one of the goals of that mockup is getting rid of the selection mode (and if we decide to go that way I am ok with it), but let's keep in mind that the mockup also works very well if we keep the selection mode. 2) We need to investigate better how the conversion tool interacts with the "scrolling arrows" shown when the number of clocks is > 3. I fear that if you have more than three clocks and you want to compare times between two that are far apart, it could be annoying. If we decide to keep the "selection mode" (see point 1) we could first select and then have a "convert" action. 3) (trivial) I think I would prefer an explicit "2 hours ahead" label than "+2" 4) Today we have a "standalone mode" when you click on a clock which is indeed not very useful and also quite ugly visually. In the mockups that is gone, and I am ok with it. However would could also think to bring back that feature in a nicer way: for instance I think it would be a nice hidden feature if swiping on one of the vertical tiles from bottom to top you could disclose some extra information like sunset and sunrise or even other "interesting trivia" (we have a RFE open asking for tide information. As I said I do not consider this a "must have" but could be one of those "nice touches" that do not get in the way of casual use, but make the app "special" 5) the mockup also includes a nicer UI for the "new" button: let's keep in mind that right now that is not implementable using libgweather as we do today. We would need to either extend libgweather or change how we retrieve the list of the cities. I do not consider changing the UI of "new" a blocker to implement the rest of the mockup
(In reply to comment #4) > (In reply to comment #1) > (...) > "the disposition of world clocks should reflect the geographic disposition" > > in other words I mean that if I am in Europe I expect US times to be on the > left and Japan times to be on the right. This is one of the pain points of the > current design. I'd not use the geographic position as a metric but the time. So the shown so the time shift gets bigger from left to right. The result is probably the same but the definition is clearer IMO. Added to the wiki page. > (...) > 2) We need to investigate better how the conversion tool interacts with the > "scrolling arrows" shown when the number of clocks is > 3. I fear that if you > have more than three clocks and you want to compare times between two that are > far apart, it could be annoying. If we decide to keep the "selection mode" (see > point 1) we could first select and then have a "convert" action. Agreed. That seems like a nifty problem to me, it's definitely worth thinking about the selection mode proposal IMO. But I'm not sure if this should have a major influence on our design. Keep in mind that I'd like to keep clocks resizeable so the solution for your usecase would be to maximize the window, I can imagine on the average laptop monitor we get at least five or so clocks with the design we have in mind. In addition I'm not a fan of having no scrollbar - agreed, a horizontal mouse drag is not the most ergonomic thing to do. So I'm in favor of having a scrollbar plus these scrolling arrows. This should make this a bit more comfortable for users with a 4-way-scroll wheel or touchpad on a laptop. > 3) (trivial) I think I would prefer an explicit "2 hours ahead" label than "+2" Changed in my version of the mockup although it doesn't make any difference to me. > 4) Today we have a "standalone mode" when you click on a clock which is indeed > not very useful and also quite ugly visually. In the mockups that is gone, and > I am ok with it. However would could also think to bring back that feature in a > nicer way: for instance I think it would be a nice hidden feature if swiping on > one of the vertical tiles from bottom to top you could disclose some extra > information like sunset and sunrise or even other "interesting trivia" (we have > a RFE open asking for tide information. As I said I do not consider this a > "must have" but could be one of those "nice touches" that do not get in the way > of casual use, but make the app "special" Agreed. Worth to keep in mind. > 5) the mockup also includes a nicer UI for the "new" button: let's keep in mind > that right now that is not implementable using libgweather as we do today. We > would need to either extend libgweather or change how we retrieve the list of > the cities. I do not consider changing the UI of "new" a blocker to implement > the rest of the mockup Thats sad, that was one of the best things for me. I'd at least like to have it in mind for later.
Created attachment 303699 [details] [review] Patch with grid view
Created attachment 303700 [details] [review] Adding changes to grid-view and design
Created attachment 303701 [details] [review] patch with overlays
Created attachment 303702 [details] [review] detecting resizing of window in widgets.vala
Created attachment 303703 [details] [review] Shifted to GtkBox view from GtkGrid for simpler implementation.
Created attachment 303704 [details] [review] Close image click detection
Created attachment 303705 [details] [review] Deleting a location - functionality added.
Created attachment 303706 [details] [review] One approach for resizing implementation
Created attachment 303707 [details] [review] Added resizing functionality
Created attachment 303708 [details] [review] Added standalone view mode that was missing
Created attachment 303709 [details] [review] A simple remove_items function call change.
Created attachment 303710 [details] [review] Fixing issues - no margin and resizing call on window state change.
Created attachment 303711 [details] [review] Setting expand properties of quit image and its container to false - not working.
Created attachment 303712 [details] [review] removing unnecessary function call that was for selection mode
There are still 2 problems after all the above patches: 1> GtkEventBox containing close button is horizontally expanded and I'm not able to make it wrap the close button image. 2> The width for a single tile is not exactly as (window_width)/number_of_locations. I'm not able to figure out how to manipulate it so that it fits perfectly in the screen and no margin is left there between the tiles.
Created attachment 304369 [details] [review] New design with Gtkgrid
Created attachment 304371 [details] [review] Detecting window resizing and setting tile width and height accordingly
Created attachment 304373 [details] [review] Used GtkBox view instead of Gtk.Grid view and used close button to delete location but close button's click is detected from a row that contains close button image which is not solved yet.
Created attachment 304374 [details] [review] Added standalone mode and removed unnecessary changes.
Review of attachment 304369 [details] [review]: commit message doesn't use present tense and isn't well formed. Could you please read http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/#how-to-write-good-commit-messages ? ::: data/css/gnome-clocks.css @@ +24,3 @@ +.dark-stripe { + background-color: rgba(0, 0, 0, 0.4); + color: white; I'd love to avoid hardcoded colors, are there any theme colors we can use to make sure this works well across other themes? This applies to other colors below and above too. @@ +36,3 @@ +.time-label { + font-size: 32px; +.light-stripe { I'd recommend making an own commit for just adding the new css stuff with a message like: css: Add new classes Those classes are part of a refactoring and will be used later for the new design. ::: src/widgets.vala @@ +447,3 @@ +/*---------------------------------------------------------------------------------*/ +private class GridView : Gtk.Grid { Before adding a whole new class and then changing everything I'd suggest to first do an own commit to remove the selection mode from everything. This allows you to just do that cleanly without any side effects. Without that I fear we might end up having loose ties somewhere. Also as I mentioned before a Gtk.Box should suffice, we only need a one dimensional list, right? @@ +451,3 @@ + NORMAL, + SELECTION + } we don't have selection mode here anymore do we? @@ +485,3 @@ + //item_grid.attach (new Gtk.Image.from_pixbuf (pixbuf), 0, 2, 1, 1); + //item_grid.attach (name, 0, 3, 1, 1); + please remove commented code @@ +496,3 @@ + details_grid.attach (textl, 0, 0, 1, 1); + details_grid.attach (subtextl, 0, 1, 1, 1); + string css_class; so here you manually compose a child every time, can't you define one item in a UI file? @@ +507,3 @@ + } else { + context.add_class ("dark-stripe"); + //textl.set_line_wrap (true); yeah, this condition shouldn't be needed with themed colors
Review of attachment 304371 [details] [review]: So this is actually a good size for a commit. The commit message is still not in imperative present tense but a commit just for forwarding the resize signal is good! ::: src/widgets.vala @@ +621,3 @@ + public void window_size_changed () { + print ("size changed\n"); hey this is a debug statement, we don't want that in production code, right? So this chunk is of no use to our users, right? @@ +622,3 @@ + public void window_size_changed () { + print ("size changed\n"); + } So there was a signal with which you could theoretically detect the resizing here directly without interference with the parent classes. Did you find out why this didn't work? I'd rather not accept a solution as long as we don't know why the superior, easier one isn't applicable.
I'll add some (fairlt generic) comments on the code as well: - the code should go in world.vala and not in widgets.vala since it is specific to world - it should use more .ui files instead of building widgets manually (I have not 100% checked this is possible) - some things of the old code got lost, e.g. keyboard handling for "escape" to exit the standalone mode - I hate "decorative" comments like /*----------------------------------*/ :-) - I think it should be split in two widgets, one for each tile and one for the container (I also have not checked this in detail)
Created attachment 305235 [details] [review] Add ContentViewWorld class in widgets.vala
Created attachment 305236 [details] [review] Add LocationTile UI template to show location in world component
Created attachment 305237 [details] [review] Add BoxView to show all the location-tiles in world component
Created attachment 305238 [details] [review] Manage location-tiles resizing on window resizing. With this approach, the resizing is working fine on adding and removing locations but on resizing the window, the tiles are resized with some delay.
Created attachment 305240 [details] [review] Add geolocation symbol for geolocation in location-tile UI file.
Created attachment 305762 [details] [review] Add locationtile UI
Created attachment 305763 [details] [review] Add css changes regarding locationtile UI
Created attachment 305764 [details] [review] Add LocationTile class to initialize locationui template for locations
Created attachment 305766 [details] [review] Add ContentViewWorld class to show locations in locationtile templates in world
Created attachment 305946 [details] [review] Add locationtile UI
Created attachment 305947 [details] [review] Add css changes regarding locationtile UI
Created attachment 305948 [details] [review] Add LocationTile class to initialize locationui template for locations
Created attachment 305949 [details] [review] Add ContentViewWorld class to show locations in locationtile templates in world
Created attachment 306320 [details] [review] Add left and right arrows for scrolling in world component
Created attachment 306322 [details] [review] Add left and right arrows for scrolling in world component
Review of attachment 305946 [details] [review]: ::: data/ui/locationtile.ui @@ +50,3 @@ + <property name="border-width">32</property> + <child> + <object class="GtkLabel" id="textl"> the id could probably rather be `time_label`, would be better to understand right? @@ +53,3 @@ + <property name="visible">True</property> + <style> + <class name="time-label"/> This class doesn't exist in this commit, you probably want to switch the order of this and the next one? @@ +64,3 @@ + </child> + <child> + <object class="GtkLabel" id="city_namel"> how about simply `city_label`? @@ +79,3 @@ + </child> + <child> + <object class="GtkLabel" id="country_namel"> you know the game :) @@ +90,3 @@ + </child> + <child> + <object class="GtkLabel" id="subtextl"> timeshift_label ?
Review of attachment 305947 [details] [review]: lgtm, needs to be switched with the previous one as indicated. Acknowledgement from pbor needed.
Review of attachment 305948 [details] [review]: ::: src/widgets.vala @@ +745,3 @@ + public Gtk.Frame details_frame; + + public Gtk.Label textl; this line's a bit long. Although clocks has no well defined codestyle one never should exceed 120 chars. Try the following commands to find such issues in general: sudo pip install coala coala # In clocks root directory, config already present One issue is present on master though... @@ +754,3 @@ + geolocation_symbol.show (); + } else { + public Gtk.Label city_namel; yeah, I'd again rename subtext with timeshift_text or so
Review of attachment 305949 [details] [review]: not fully reviewed yet but I'm away for some time so if you want you can already start reworking :) ::: src/widgets.vala @@ +841,3 @@ + + if (number_of_locations != 0) { + if (window_width != width || window_height != height ) { why don't you take the int conversion of the division directly and then take the int max function? @@ +854,3 @@ + Gdk.Pixbuf? pixbuf = location_tile.weather_image.get_pixbuf (); + int width, height; + } you can calculate that outside the loop, only do it once. @@ +856,3 @@ + calculate_location_tile_size (out width, out height); + if (width > 0 && height > 0) { + so wouldn't it be cheaper (and thus faster and thus avoiding graphical glitches) if we just cropped the image instead of actually interpolating things? pbor, any suggestions? ::: src/world.vala @@ +128,3 @@ + return _("No time difference"); + } + result = _("%.1f hours ahead".printf (hours)); you could add this method in an own commit since it's isolated. Makes review of this one simpler. @@ +169,2 @@ } + css_class = "stripe"; the change of the css class belongs into https://bugzilla.gnome.org/review?bug=742406&attachment=305947 don't you think?
Review of attachment 306322 [details] [review]: commit message shortlog is too long, git will cut it, can you come up with something shorter? ::: src/widgets.vala @@ +773,3 @@ private Gtk.ListStore model; + private double window_width; + private double window_height; why changing from int to double? It will only have int values rright? @@ +843,3 @@ + right_overlay.add_overlay(right_button); + left_overlay.add(right_overlay); + left_most_loc = left_most_loc - 1; maybe extract this into a ui file? This grows big and ugly. @@ +918,3 @@ + return animate(target, start, end); + }); + right_button.show (); in the commit message you state you add the buttons. You seem to add the animation too -> own commit.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/gnome-clocks/-/issues/ Thank you for your understanding and your help.