GNOME Bugzilla – Bug 785675
Use ui template for ProcessRow, SubProcessSubRow and SubProcessListBox
Last modified: 2017-11-24 13:36:26 UTC
Add ui template for ProcessRow, SubProcessSubRow and SubProcessListBox.
Created attachment 356705 [details] [review] process-list: Add ui template & better refreshing Add ui template for ProcessRow, SubProcessSubRow and SubProcessListBox. This do better refreshing SubProcessListBox too.
Review of attachment 356705 [details] [review]: Make multiple patches. Each one of ProcessRow, SubProcessSubRow, SubProcessListBox could be its own patch. The refreshing bit should be another one. The more patches the better. Try to make the changes as small as possible, so we can later git blame the patches.
Created attachment 356931 [details] [review] Splitted patches to 4 commits.
Created attachment 356932 [details] [review] sub-process-list-box: Port to Gtk+ widget template
Created attachment 356933 [details] [review] process-row: Port to Gtk+ widget template
Created attachment 356934 [details] [review] process-list: Better refreshing process list We need more frequent refreshing process list for example for disk i/o. For better controllability we refreshing all list with processes every 5sec. But list with subprocesses and values in process-row/sub-row every 1sec.
Created attachment 356935 [details] [review] sub-process-sub-row: Port to Gtk+ widget template
Splitted patches to 4 commits.
Review of attachment 356932 [details] [review]: This patch doesn't seem to apply any longer. Could you please rebase it? ::: data/ui/sub-process-list-box.ui @@ +1,3 @@ +<?xml version="1.0" encoding="UTF-8"?> +<interface> + <requires lib="gtk+" version="3.10"/> Use <!-- interface-requires gtk+ 3.10 --> instead. ::: src/sub-process-list-box.vala @@ +39,3 @@ + } + + public void init(Process process, ProcessListBoxType type) I would expect that you'd be able to run this block within the construction method somehow (despite the gtk+ composite template).
Review of attachment 356933 [details] [review]: Thanks for your patch! ::: data/interface/adwaita-dark.css @@ +29,3 @@ +ProcessRow.opened:hover { + background: #4d5356; Have you tested Usage with the dark variant or other themes to see whether these colors work just fine? Can't we get these from the theme somehow? Check the constants defined at gtk+/gtk/theme/Adwaita/_colors-public.scss ::: src/process-row.vala @@ +53,3 @@ + class construct + { + } This could also be defined in the .ui file.
The comments regarding the first two patches also apply to the other two. Could you please address them all and re-attach? Thanks Petr!
Created attachment 360179 [details] [review] sub-process-sub-row: Port to Gtk+ widget template Use <!-- interface-requires gtk+ 3.10 --> instead of <requires lib="gtk+" version="3.10"/>
Created attachment 360180 [details] [review] sub-process-list-box: Port to Gtk+ widget template Use <!-- interface-requires gtk+ 3.10 --> instead of <requires lib="gtk+" version="3.10"/>
Created attachment 360181 [details] [review] process-row: Port to Gtk+ widget template Use <!-- interface-requires gtk+ 3.10 --> instead of <requires lib="gtk+" version="3.10"/>
Created attachment 360182 [details] [review] process-list: Better refreshing process list We need more frequent refreshing process list for example for disk i/o. For better controllability we refreshing all list with processes every 5sec. But list with subprocesses and values in process-row/sub-row every 1sec.
Created attachment 360183 [details] [review] css-styling: Update css for better support others themes Swap using css-name for css class, for better support other themes. It separate adwaita.css and adwaita-dark.css to common.css for common items.
>::: src/sub-process-list-box.vala > @@ +39,3 @@ > + } > + > + public void init(Process process, ProcessListBoxType type) > > I would expect that you'd be able to run this block within the construction method somehow (despite the gtk+ composite template). I don't have any idea how can I use constructor with parameters with gtk+ composite template.
I've done some modifications and refactoring, moving code into more methods.. I'm attach an updated patches.
Created attachment 360448 [details] [review] sub-process-sub-row: Port to Gtk+ widget template Updated version.
Created attachment 360449 [details] [review] sub-process-list-box: Port to Gtk+ widget template Updated version.
Created attachment 360450 [details] [review] process-row: Port to Gtk+ widget template Updated version.
Created attachment 360451 [details] [review] process-list: Better refreshing process list We need more frequent refreshing process list for example for disk i/o. For better controllability we refreshing all list with processes every 5sec. But list with subprocesses and values in process-row/sub-row every 1sec. Updated version.
Created attachment 360452 [details] [review] css-styling: Update css for better support others themes Swap using css-name for css class, for better support other themes. It separate adwaita.css and adwaita-dark.css to common.css for common items. Updated version.
Review of attachment 360448 [details] [review]: ::: data/interface/adwaita.css @@ +37,1 @@ background: #e2e2e2; Couldn't these values come from the theme?
(In reply to Felipe Borges from comment #24) > Review of attachment 360448 [details] [review] [review]: > > ::: data/interface/adwaita.css > @@ +37,1 @@ > background: #e2e2e2; > > Couldn't these values come from the theme? It would be nice, but how? I did not find these colors defined in adwaita theme..
#e2e2e2 = (In reply to Petr Štětka from comment #25) > (In reply to Felipe Borges from comment #24) > > Review of attachment 360448 [details] [review] [review] [review]: > > > > ::: data/interface/adwaita.css > > @@ +37,1 @@ > > background: #e2e2e2; > > > > Couldn't these values come from the theme? > > It would be nice, but how? I did not find these colors defined in adwaita > theme.. Where did you get these colors from in the first place? #e2e2e2 seems to be very close to @theme_bg_color. I assume you used a color picker to get it from the mockups? In this case, try to get similar colors from the theme. I am not sure whether the designers are sketching mockups with the exact same theme colors. I am being picky about these colors because that's how Usage looks like in: - Adwaita with the dark variant https://i.imgur.com/qjwGLVE.png - HighContrast https://i.imgur.com/ycCK9TG.png notice that the main view background is not white? This is a problem for users with visual disabilities. - Imagine how dissonant it will look like in Ubuntu 17.10 (huge GNOME user base) http://www.omgubuntu.co.uk/2017/09/ubuntu-17-10-gnome-shell-ambiance
(In reply to Felipe Borges from comment #26) > #e2e2e2 = (In reply to Petr Štětka from comment #25) > > (In reply to Felipe Borges from comment #24) > > > Review of attachment 360448 [details] [review] [review] [review] [review]: > > > > > > ::: data/interface/adwaita.css > > > @@ +37,1 @@ > > > background: #e2e2e2; > > > > > > Couldn't these values come from the theme? > > > > It would be nice, but how? I did not find these colors defined in adwaita > > theme.. > > Where did you get these colors from in the first place? > > #e2e2e2 seems to be very close to @theme_bg_color. I assume you used a color > picker to get it from the mockups? In this case, try to get similar colors > from the theme. I am not sure whether the designers are sketching mockups > with the exact same theme colors. > > I am being picky about these colors because that's how Usage looks like in: > > - Adwaita with the dark variant https://i.imgur.com/qjwGLVE.png > - HighContrast https://i.imgur.com/ycCK9TG.png > notice that the main view background is not white? This is a problem for > users with visual disabilities. > - Imagine how dissonant it will look like in Ubuntu 17.10 (huge GNOME user > base) http://www.omgubuntu.co.uk/2017/09/ubuntu-17-10-gnome-shell-ambiance I think due to https://gitlab.gnome.org/GNOME/gnome-usage/merge_requests/10, we can not solve these colors in this bug (here on bugzilla) but we can focus on these patches as port to Gtk+ template file and Better refreshing process list. It would be nice to soon resolve bugs on bugzilla due to move to gitlab.
(In reply to Petr Štětka from comment #27) > I think due to https://gitlab.gnome.org/GNOME/gnome-usage/merge_requests/10, > we can not solve these colors in this bug (here on bugzilla) but we can > focus on these patches as port to Gtk+ template file and Better refreshing > process list. > It would be nice to soon resolve bugs on bugzilla due to move to gitlab. Ok. Could you please remove the CSS changes from the patch so it doesn't create unnecessary merge conflicts with that merge-request in gitlab? Thanks!
Created attachment 362184 [details] [review] sub-process-sub-row: Port to Gtk+ widget template
Created attachment 362185 [details] [review] sub-process-list-box: Port to Gtk+ widget template
Created attachment 362186 [details] [review] process-row: Port to Gtk+ widget template
Created attachment 362187 [details] [review] process-list: Better refreshing process list We need more frequent refreshing process list for example for disk i/o. For better controllability we refreshing all list with processes every 5sec. But list with subprocesses and values in process-row/sub-row every 1sec.
(In reply to Felipe Borges from comment #28) > (In reply to Petr Štětka from comment #27) > > I think due to https://gitlab.gnome.org/GNOME/gnome-usage/merge_requests/10, > > we can not solve these colors in this bug (here on bugzilla) but we can > > focus on these patches as port to Gtk+ template file and Better refreshing > > process list. > > It would be nice to soon resolve bugs on bugzilla due to move to gitlab. > > Ok. Could you please remove the CSS changes from the patch so it doesn't > create unnecessary merge conflicts with that merge-request in gitlab? > > Thanks! Yes, done.
Review of attachment 362184 [details] [review]: Thank you!
Review of attachment 362185 [details] [review]: ok
Review of attachment 362186 [details] [review]: sure
Attachment 362184 [details] pushed as 4a72be0 - sub-process-sub-row: Port to Gtk+ widget template Attachment 362185 [details] pushed as f21c39b - sub-process-list-box: Port to Gtk+ widget template Attachment 362186 [details] pushed as a140ee7 - process-row: Port to Gtk+ widget template
SubProcessListBox is no more used in master.