After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 780778 - Improve media selector
Improve media selector
Status: RESOLVED FIXED
Product: gnome-games
Classification: Core
Component: general
3.24.x
Other Linux
: Normal enhancement
: ---
Assigned To: GNOME Games maintainers
GNOME Games maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-31 17:55 UTC by Abhinav Singh
Modified: 2017-04-23 00:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ui: Use radio buttons for media selector (4.34 KB, patch)
2017-03-31 18:25 UTC, Abhinav Singh
none Details | Review
Using builder preferences (listbox) style (6.29 KB, image/png)
2017-04-10 05:21 UTC, Abhinav Singh
  Details
Using text radio buttons (6.24 KB, image/png)
2017-04-10 05:26 UTC, Abhinav Singh
  Details
ui: Reformat preferences-switch-item (7.03 KB, patch)
2017-04-12 13:54 UTC, Abhinav Singh
committed Details | Review
ui: Improve media-selector (3.97 KB, patch)
2017-04-12 13:55 UTC, Abhinav Singh
none Details | Review
ui: Use checkmark in MediaSelector (4.09 KB, patch)
2017-04-22 01:50 UTC, Abhinav Singh
committed Details | Review

Description Abhinav Singh 2017-03-31 17:55:52 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.
Comment 1 Abhinav Singh 2017-03-31 18:25:24 UTC
Created attachment 349072 [details] [review]
ui: Use radio buttons for media selector
Comment 2 Adrien Plazas 2017-04-07 11:49:31 UTC
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?
Comment 3 Abhinav Singh 2017-04-10 05:21:38 UTC
Created attachment 349581 [details]
Using builder preferences (listbox) style
Comment 4 Abhinav Singh 2017-04-10 05:26:17 UTC
Created attachment 349582 [details]
Using text radio buttons
Comment 5 Abhinav Singh 2017-04-12 13:54:52 UTC
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.
Comment 6 Abhinav Singh 2017-04-12 13:55:50 UTC
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.
Comment 7 Adrien Plazas 2017-04-18 05:15:46 UTC
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.
Comment 8 Adrien Plazas 2017-04-18 05:15:57 UTC
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.
Comment 9 Adrien Plazas 2017-04-18 05:19:53 UTC
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).
Comment 10 Adrien Plazas 2017-04-18 05:19:57 UTC
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).
Comment 11 Abhinav Singh 2017-04-18 07:09:18 UTC
(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 ---------------------
Comment 12 Adrien Plazas 2017-04-19 07:25:54 UTC
(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.
Comment 13 Adrien Plazas 2017-04-19 07:27:19 UTC
Review of attachment 349732 [details] [review]:

The Bugzilla review tool lead me into error: your patch is good. :)
Comment 14 Abhinav Singh 2017-04-22 01:50:55 UTC
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.
Comment 15 Adrien Plazas 2017-04-22 12:49:02 UTC
Review of attachment 350217 [details] [review]:

LGTM.
Comment 16 Abhinav Singh 2017-04-23 00:28:40 UTC
Attachment 349732 [details] pushed as abb916e - ui: Reformat preferences-switch-item
Attachment 350217 [details] pushed as 06dd9dd - ui: Use checkmark in MediaSelector