GNOME Bugzilla – Bug 780778
Improve media selector
Last modified: 2017-04-23 00:28:50 UTC
Media selector should use 'row-activated' instead of 'row-selected'. Currently, just highlighting a row, selects the media and changes it. We should wait for the user to actually click or enter the highlighted media.
Created attachment 349072 [details] [review] ui: Use radio buttons for media selector
Mmmh maybe it's better, I'm not sure yet. Is there some recommendations regarding that on the HIG, does another application use a similar design or does any designer recommended this change?
Created attachment 349581 [details] Using builder preferences (listbox) style
Created attachment 349582 [details] Using text radio buttons
Created attachment 349732 [details] [review] ui: Reformat preferences-switch-item Use properties instead of methods and update their corresponding uses. Rename preferences-switch-item to checkmark-item.
Created attachment 349733 [details] [review] ui: Improve media-selector Enhance media selection with checkmark-item, and use 'row-activated' signal in place of 'row-selected' signal. Also, reformat code to avoid the use of row_to_int hashtable.
Review of attachment 349733 [details] [review]: Globally the patch is very good, though some details need more work. "Improve" and "enhance" are not objective terms, you want to describe what the commit does from an objective POV, you can if you want be subjective by trying to explain why we want the change, how ot makes the software better. Fox example: """ ui: Use a checkmark in MediaSelector Highlight the selected media with a checkmark in MediaSelector, following the recommendations from the HIG. """ It would be nice to also specify that it solves the problem of changing the disc on selection and not activation. ::: src/ui/media-selector.vala @@ +38,3 @@ + var checkmark_item = new CheckmarkItem (media_name); + var current_media = (_media_set.selected_media_number == media_number); "current_media" sounds like the variable is a media, as it'as a boolean value "is_current_media" would be more explicit.
Review of attachment 349732 [details] [review]: "preferences-switch-item.ui" should be renamed into "checkmark-item.ui" too (looks like you forgot a `git mv` or something because you fixed it in the makefile).
(In reply to Adrien Plazas from comment #10) > Review of attachment 349732 [details] [review] [review]: > > "preferences-switch-item.ui" should be renamed into "checkmark-item.ui" too > (looks like you forgot a `git mv` or something because you fixed it in the > makefile). The rename for ui files is done. I don't understand that. ...preferences-switch-item.ui => checkmark-item.ui} | 4 ++-- I think you mean vala files which seem to be deleted and added, instead of renaming? src/ui/checkmark-item.vala | 21 +++++++++++++++++++++ src/ui/preferences-switch-item.vala | 21 ---------------------
(In reply to Abhinav Singh from comment #11) > (In reply to Adrien Plazas from comment #10) > > Review of attachment 349732 [details] [review] [review] [review]: > > > > "preferences-switch-item.ui" should be renamed into "checkmark-item.ui" too > > (looks like you forgot a `git mv` or something because you fixed it in the > > makefile). > > The rename for ui files is done. I don't understand that. > ...preferences-switch-item.ui => checkmark-item.ui} | 4 ++-- Sorry, the Bugzilla review tool seems to be buggy and doesn't show file renamings. :) It is indeed done as expected.
Review of attachment 349732 [details] [review]: The Bugzilla review tool lead me into error: your patch is good. :)
Created attachment 350217 [details] [review] ui: Use checkmark in MediaSelector Highlight the current media with a checkmark in MediaSelector following the recommendations from the HIG. Solve the problem of changing disc on selection by using 'row-activated' signal in place of 'row-selected' signal. Also, reformat code to avoid the use of row_to_int hashtable.
Review of attachment 350217 [details] [review]: LGTM.
Attachment 349732 [details] pushed as abb916e - ui: Reformat preferences-switch-item Attachment 350217 [details] pushed as 06dd9dd - ui: Use checkmark in MediaSelector