GNOME Bugzilla – Bug 749158
Various UI improvements
Last modified: 2017-09-19 07:17:02 UTC
Created attachment 303130 [details] [review] Various UI improvements Continue my efforts after bug 747729 ( https://bugzilla.gnome.org/show_bug.cgi?id=747729 ). Things that are improved: main shell, calendar views, dialogs, dark theme variant, ...
Created attachment 303131 [details] Mail view screenshot
Created attachment 303132 [details] Calendar day view screenshot
Created attachment 303133 [details] Work week calendar view screenshot
Created attachment 303134 [details] Calendar week view screenshot
Created attachment 303135 [details] Calendar month view screenshot
Created attachment 303136 [details] Calendar list view screenshot
Created attachment 303137 [details] Mail view dark screenshot
Created attachment 303138 [details] Day view dark screenshot
Created attachment 303139 [details] Calendar work week view dark screenshot
Created attachment 303140 [details] Calendar week view dark screenshot
Created attachment 303141 [details] Calendar week view dark screenshot
Created attachment 303142 [details] Calendar month view dark screenshot
Created attachment 303143 [details] Calendar list view dark screenshot
Created attachment 303144 [details] Compose message dialog screenshot
Created attachment 303145 [details] Appointment dialog screenshot
Thanks for a bug report and patch. I didn't read it yet, only compiled. I see these compiler warnings: --------------------------------------------------------------------------- e-table-click-to-add.c: In function 'etcta_style_updated': e-table-click-to-add.c:82:19: warning: unused variable 'text' [-Wunused-variable] GdkColor fg, bg, text; ^ e-table-click-to-add.c: In function 'create_rect_and_text': e-table-click-to-add.c:261:19: warning: unused variable 'text' [-Wunused-variable] GdkColor fg, bg, text; ^ e-day-view-main-item.c: In function 'day_view_main_item_draw_day_event': e-day-view-main-item.c:202:10: warning: variable 'date_fraction' set but not used [-Wunused-but-set-variable] gdouble date_fraction; ^ e-day-view-main-item.c:193:39: warning: variable 'bar_y1' set but not used [-Wunused-but-set-variable] gint item_x, item_y, item_w, item_h, bar_y1, bar_y2; ^ e-day-view-main-item.c: At top level: e-day-view-main-item.c:603:1: warning: 'day_view_main_item_draw_events_in_vbars' defined but not used [-Wunused-function] day_view_main_item_draw_events_in_vbars (EDayViewMainItem *main_item, ^ --------------------------------------------------------------------------- None should be there. The scary one is the last, about unused function. You do not need to remove it.
Review of attachment 303130 [details] [review]: I thought some of the things I noticed in your screen shots are due to your theme, but I see they are not (I miss a comparision in the screen shots, have "before and after" states beside each other. So: a) I do not like circles in the mini calendar b) the flat events look odd as well c) add an alarm into the event and see what it looks like in various views d) the tool bar looks too tiny, like not following my theme e) colors in the day/work week views are not good in the "all day events" area, it is not distinguishable There might be probably more things, I noticed only these on the first look. All these changes should be properly discussed first, they are not like the really small UI changes from your previous bug report.
Yes, i just test on GNOME theme (Adwaita). What is your theme ?
I do not use gnome-shell, thus my theme is from a different desktop. It's called BlueMenta. Anyway, this is not about used theme, I just did think it is, but it isn't.
I just tested BlueMenta theme a) What shape do you like ? b) ... c) ... Look good for me :) d) I set toolbar icon size to small because toolbar have larger padding in GNOME, i will revert this change if needed. e) What is your suggestion ?
Created attachment 303248 [details] (Before patch) Calendar week view screenshot
Created attachment 303249 [details] (Before patch) Calendar week view screenshot 2
(In reply to Trinh Anh Ngoc from comment #3) > Created attachment 303133 [details] > Work week calendar view screenshot (In reply to Trinh Anh Ngoc from comment #21) > Created attachment 303248 [details] > (Before patch) Calendar week view screenshot Compare these two pictures. I see a difference in the third+ column (Wen 25) in the tiny column on the left. (In reply to Trinh Anh Ngoc from comment #20) > a) What shape do you like ? I like the way it was. > b) ... It can be that some people like flat view, while other (like me) the shaded view. It's a good candidate for an option. > c) ... Look good for me :) It's slightly off the event on the left for me. > d) I set toolbar icon size to small because toolbar have larger padding in > GNOME, i will revert this change if needed. Please do revert that change. > e) What is your suggestion ? Have the "all days events" area distinguishable, like it is before your change. Otherwise it looks like an inactive area where users cannot do anything, which is not true. I know I wanted from you to merge multiple small patches into one patch in bug #747729, the reason was that the consolidated patch was still small, but it's not the case here (the patch here is like 10 times bigger than the previous one), thus I'd prefer to split it into logic parts (and eventually bug reports, but not necessarily). You cannot hide so many UI changes into one "Various UI improvements" commit, it'll be better to name them. We can use the points from the above (a)..e)), and/or add more of them. Feel free to suggest the parts, then you can split and enhance the patch as we'll agree on it. Just to not have (only) a negative impression from my response, the sharper drawing of the calendar views looks pretty good, more modern with your change.
Ok, i split into 3 patches: 1. Layout improvements (margins, borders, etc) 2. Modernize mini calendar 3. Modernize calendar view => Evolution UI looks good now :)
Created attachment 303545 [details] [review] Layout improvements
Created attachment 303546 [details] [review] Modernize mini calendar
Created attachment 303547 [details] [review] Modernize calendar view
Screenshot http://i61.tinypic.com/jb67fp.png Left: before Right: after
Sorry, full size : http://postimg.org/image/3ld0lxf73/full
Thanks for the update. From the screen shot: a) the composer's text area lost its border, which looks odd (also comparing to your effort to add borders on other places, like toolbars). Similar with the attachment panel's content there. b) the all-day area is still almost the same color as inactive border (I did mention it earlier), it should be better distinguishable that the are can do something (it's here only slightly lighter, which is hard to recognize) c) mini-calendar - do not use circles, but squares, please (also mentioned earlier) d) work-week doesn't highlight "Event B" time in the left thin column, like for example the Event A is highlighted (from that I suppose the patches still claim compile-time warnings, unless you removed the function which should stay there and be used instead). e) if I see correctly then you did not add an option to use shaded or non-shaded event drawings into the Calendar Preferences, right?
Created attachment 303578 [details] [review] Layout improvements Added borders to compose message text box.
Created attachment 303579 [details] [review] Modernize mini calendar Curved rectangle
Created attachment 303580 [details] Compose message screenshot
Created attachment 303581 [details] Compose message screenshot
Created attachment 303582 [details] Mini calendar screenshot
Created attachment 303602 [details] [review] Calendar view improvements I only tweak a bit on current calendar view to fit to rest of ui.
Created attachment 303603 [details] Calendar view screenshot
(In reply to Trinh Anh Ngoc from comment #35) > Created attachment 303582 [details] > Mini calendar screenshot The main thing about the mini calendar, from my point of view, is that when multiple days are selected, then it is a continuous interval, not a single-selected set of days. Similar to selection of a text, it would look awkward to have empty gaps between letters. As I said, it's just my point of view. Alan, what do you think, please?
(In reply to Trinh Anh Ngoc from comment #36) > Created attachment 303602 [details] [review] [review] > Calendar view improvements > > I only tweak a bit on current calendar view to fit to rest of ui. See the Day View, where the icons are too high, looking out of alignment for the first line of the text. Some cases can draw the icons not horizontally, but vertically instead. That's when the event is taking long enough and there are many icons (turn on categories for the event). Still, I'm fine with flat events, if it's an option. If it's too hard (unnecessary code duplication or code maintenance needed), then let's do only this change.
Review of attachment 303578 [details] [review]: This one looks fine, thus I committed it. The other two patches here are under discussion. Created commit c0761d4 in evo master (3.17.3+)
Trinh, will you have time to work on the above requests, or I should just close this bug as done, please?
Created attachment 310622 [details] [review] Fix mini calendar button style on hover
Created attachment 310623 [details] [review] Fix EMailConfigAssistant UI
Created attachment 310624 [details] Screenshot - Mini calendar buttons on hover
Created attachment 310625 [details] Screenshot - EMailConfigAssistant
Thanks for the update. I'll review this later, evolution is currently under UI freeze, thus this would be committed for ~3.19.1.
Review of attachment 310622 [details] [review]: Apart of the below question looks fine. ::: shell/e-shell-window.c @@ +123,3 @@ +}\ +ECalendar .button:hover {\ + color: red;\ What about using color: @theme_selected_bg_color; instead of the hard-coded red color?
Review of attachment 310623 [details] [review]: Needs-work due to the memory leak. ::: mail/e-mail-config-assistant.c @@ +1137,3 @@ + + action_area = gtk_buildable_get_internal_child (GTK_BUILDABLE (assistant), + gtk_builder_new (), This leaks the GtkBuilder instance @@ +1140,3 @@ + "action_area"); + if (action_area) + gtk_container_set_border_width (GTK_CONTAINER (action_area), 12); Is it a good idea to change anything on this internal object? I see it is documented on the GtkAssistant object, but as it's an internal child, it looks like not being good to change. Also, the change can cause a diverge from the theme settings, if any overrides this border size.
Created attachment 311330 [details] [review] Fix mini calendar button style on hover Use @theme_selected_bg_color instead of hard-coded color
Created attachment 311332 [details] [review] Fix EMailConfigAssistant UI Fix memory leak AFAIK, GtkAssistant action area border cannot be changed by theme
Created attachment 311425 [details] [review] Tweak EMailConfigAssistant UI
Thanks for a quick update. The patches looks fine, and if there is not any better option, then let's have the GtkAssistant code there. I noticed that gtk3 3.16.x has its own border on the action area, while to be 3.18.0 doesn't. It's no big deal. I'll commit the patches (probably as one) after the hard code freeze. Thanks again for our help with this.
(In reply to Milan Crha from comment #52) > Thanks again for our help with this. s/our/your/
I merged the two patches into one commit: Created commit d1990b8 in evo master (3.19.1+)
*** Bug 761493 has been marked as a duplicate of this bug. ***
Just for your information, with commit 9ce1a78148 (3.27.1+) the events are drawn as flat by default in the Calendar views, with added option to change it: $ gsettings set org.gnome.evolution.calendar draw-flat-events false