GNOME Bugzilla – Bug 779165
Thumbnails > "Only for files smaller than" drop down should be a spin button instead of hard-coded values
Last modified: 2017-08-17 13:59:38 UTC
Created attachment 346618 [details] Image of change to preferences box Replace drop down hard-coded values with a spin box. Reasoning: File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. So currently, for the price of 2MB, the user must allow thumbnails up to 100MB. To save wasted processor time, this should be a user-definable threshold that is suited to the user's own needs. This is particularly the kind of situation that the spin box was designed for! :) In this example, the user could set the threshold to 15MB and save the (potentially massive) amount of processor time used to thumbnail larger files. Thanks for your consideration and help! :) -C
Yeah, no idea why is a combobox :D I think this is also good for newcomers.
Created attachment 346659 [details] [review] changed dropdown to spinbar I was able to change the ui. But I am not able to make the spinbar responsive to increment or decrement.
Review of attachment 346659 [details] [review]: Hi, thanks for working on this! Your patch does not apply, however. This link contains information on how to prepare patches: https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Submitting_patches You should also not add libgd/ without a good reason. ::: src/nautilus-properties-window.c @@ +1513,3 @@ inconsistent_string, show_original, + FALSE); Why this change? ::: src/resources/ui/nautilus-preferences-window.ui @@ +1083,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="label" translatable="yes">Onl_y for files smaller than (MB) :</property> I think connecting to the “output” signal of the spin button and formatting the output there would be better. @@ -1086,2 @@ <property name="use_underline">True</property> - <property name="mnemonic_widget">preview_image_size_combobox</property> This needs to stay, but with the new widget as the value. @@ +1097,3 @@ <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="text" translatable="yes">10</property> This isn’t right. The “numeric” property should probably be set to true, as non-numeric characters don’t make sense in this case.
Created attachment 346714 [details] [review] Replace drop down hard-coded values with a spin box. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox int the preferences to a spinbutton. The value is in Mega bytes, with a maximum of 4096.
The changes done to the thumbnail size limit in this are not saved, gsettings get org.gnome.nautilus.preferences thumbnail-limit Always gave me the same output.
maybe you have to build dconf?
basically if in the terminals says "using gsettings memory backend" then you need to build dconf
(In reply to Carlos Soriano from comment #7) > basically if in the terminals says "using gsettings memory backend" then you > need to build dconf When launching nautilus from terminal? No, it's not printing "using gsettings memory backend"
Can someone review my patch? I am unable to proceed with the saving of the preference.
Created attachment 346764 [details] [review] Replace drop down hard-coded values with a spin box. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox int the preferences to a spinbutton. The value is in Mega bytes, with a maximum of 4096.
Review of attachment 346764 [details] [review]: ::: src/nautilus-preferences-window.c @@ +371,1 @@ + retval = g_variant_new_uint64 (*widget_value*1024); Spin button values are doubles, not ints. @@ +401,2 @@ { + guint minimum = 10; What’s this? @@ +404,1 @@ g_settings_bind_with_mapping ( You don’t need special mapping functions, it’s just a gdouble we’re dealing with. @@ +405,3 @@ + settings, prefs, gtk_builder_get_object (builder, widget_name), "value", + G_SETTINGS_BIND_DEFAULT, spinbutton_get_mapping, spinbutton_set_mapping, + &minimum, g_free); You’re passing a pointer to a stack variable, which is just asking for trouble. ::: src/resources/ui/nautilus-preferences-window.ui @@ +4,3 @@ <requires lib="gtk+" version="3.16"/> + <object class="GtkAdjustment" id="adjustment1"> + <property name="lower">5</property> Why 5? @@ +5,3 @@ + <object class="GtkAdjustment" id="adjustment1"> + <property name="lower">5</property> + <property name="upper">4096</property> What if the value of the setting is something more? @@ +6,3 @@ + <property name="lower">5</property> + <property name="upper">4096</property> + <property name="value">10</property> Not sure if this makes sense if we’re mapping to a setting. @@ -1086,2 @@ <property name="use_underline">True</property> - <property name="mnemonic_widget">preview_image_size_combobox</property> Again, don’t remove this property. The label has a mnemonic. @@ +941,3 @@ <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="max_length">7</property> The “numeric” property is set to true and you limit the value in the adjustment. That itself limits the length. @@ +943,3 @@ + <property name="max_length">7</property> + <property name="width_chars">7</property> + <property name="input_purpose">number</property> With the “numeric” property set to true, this is redundant. @@ +1210,3 @@ + </data> + </object> + <object class="GtkListStore" id="model8"> This is no longer needed.
Created attachment 346787 [details] [review] Replace drop down hard-coded values with a spin box. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox int the preferences to a spinbutton. The value is in Mega bytes, with a maximum of 4096.
The changes done in preferences are saved in dconf. However, the thumbnails are being generated regardless of the thumbnail-limit.
(In reply to Vyas Giridhar from comment #13) > The changes done in preferences are saved in dconf. > However, the thumbnails are being generated regardless of the > thumbnail-limit. How do you know the thumbnails are being generated? (Keep in mind that generating thumbnails is a different process that showing the ones already generated and cached, the problem here is the I/O and CPU load of generating them for first time like when accessing network locations :))
(In reply to Carlos Soriano from comment #14) > (In reply to Vyas Giridhar from comment #13) > > The changes done in preferences are saved in dconf. > > However, the thumbnails are being generated regardless of the > > thumbnail-limit. > > How do you know the thumbnails are being generated? (Keep in mind that > generating thumbnails is a different process that showing the ones already > generated and cached, the problem here is the I/O and CPU load of generating > them for first time like when accessing network locations :)) I deleted the cached thumbnails at ~/.cache/thumbnails each time I changed thumbnail-limit
Created attachment 346848 [details] [review] Replace drop down hard-coded values with a spin box. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox int the preferences to a spinbutton and changes the signal handler for thumbnail-limit. The value is in MB, with a maximum of 4096 and a minimum of 1.
I have tried many values and it seems to be working.
Review of attachment 346848 [details] [review]: Hey! Good progress. The patch doesn't work for me when updating the thumbnail value in preferences, and it outputs a few criticals errors (didn't you see them when trying the patch?). Also, for the UI I would rather do the units after the spinner. The commit message title is missing the file that touches (in this case you can say preferences-window: bla bla). See https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines Looking forward to see more progress on this! ::: src/nautilus-file.c @@ +9224,1 @@ + cached_thumbnail_limit *= 1024*1024; Use a constant @@ +9372,3 @@ thumbnail_limit_changed_callback (NULL); g_signal_connect_swapped (nautilus_preferences, + "value-changed::" NAUTILUS_PREFERENCES_FILE_THUMBNAIL_LIMIT, this signal doesn't exist. Where did you get the "value-changed"? ::: src/nautilus-preferences-window.c @@ +367,3 @@ + const char *prefs) +{ + g_settings_bind ( start parameters in the first line (I know it was like this before) ::: src/resources/ui/nautilus-preferences-window.ui @@ +266,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="label" translatable="yes" context="the n-th position of an icon caption" comments="Translators: This is an ordinal number">Second</property> this change looks irrelevant @@ +281,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="label" translatable="yes" context="the n-th position of an icon caption" comments="Translators: This is an ordinal number">Third</property> ditto @@ +296,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="label" translatable="yes" context="the n-th position of an icon caption" comments="Translators: This is an ordinal number">First</property> ditto @@ +927,3 @@ <property name="can_focus">False</property> + <property name="label" translatable="yes">Onl_y for files smaller than (MB):</property> + <property name="mnemonic_widget">preview_image_size_spinbutton</property> not need to change the order of the properties either @@ +1078,3 @@ </object> </child> + <child type="titlebar"> why moving this? If it's a style change, you can do it in a different patch.
(In reply to Carlos Soriano from comment #18) > Also, for the UI I would rather do the units after the spinner. AFAIK, the output of the spin button can be formatted, so we could show the unit inside it. > why moving this? If it's a style change, you can do it in a different patch. They’re using Glade, I imagine, which doesn’t care about anything.
(In reply to Ernestas Kulik from comment #19) > (In reply to Carlos Soriano from comment #18) > > Also, for the UI I would rather do the units after the spinner. > > AFAIK, the output of the spin button can be formatted, so we could show the > unit inside it. > I don't think you can. Do you have an example? > > why moving this? If it's a style change, you can do it in a different patch. > > They’re using Glade, I imagine, which doesn’t care about anything. Oh good point, ignore my review for the ui file then
(In reply to Carlos Soriano from comment #20) > (In reply to Ernestas Kulik from comment #19) > > AFAIK, the output of the spin button can be formatted, so we could show the > > unit inside it. > > > > I don't think you can. Do you have an example? I found this in the docs: https://developer.gnome.org/gtk3/stable/GtkSpinButton.html#GtkSpinButton-output Not sure how well it would work, but it’s an idea.
Created attachment 346970 [details] [review] preferences-window: Replace Combobox with a spin box. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox in the preferences to a spinbutton and changes the signal handler for thumbnail-limit. The value is in MB, with a maximum of 4096 and a minimum of 1.
The thumbnail setting updates only for pictures, not for documents or videos.
Created attachment 346983 [details] [review] preferences-window: Replace Combobox with spinbutton for thumbnail limit. File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox in the preferences to a spinbutton and changes the signal handler for thumbnail-limit. The value is in MB, with a maximum of 4096 and a minimum of 1.
Can someone review this?
Review of attachment 346983 [details] [review]: Hey, good progress on this! Still some work needed: The commit message tittle is too long. How about "preferences-window: Use spin button for thumbnail limit". Also you skipped some review details I mentioned before. The unit format should be after the value. You can try what Ernestas mentioned with formatting the output of the spin button in order to put inside the MB. ::: src/nautilus-file.c @@ +9222,3 @@ + NAUTILUS_PREFERENCES_FILE_THUMBNAIL_LIMIT); + + const guint64 convertion_rate = 1048576; Compile time constants are put at the start of the file and in caps. Look an example in https://en.wikipedia.org/wiki/Constant_(computer_programming) Also use a more descriptive and global distinctively name like MEGA_TO_BASE_RATE @@ +9224,3 @@ + const guint64 convertion_rate = 1048576; + + //Converts the obtained limit in MB to Bytes bytes, the MB is in caps because it's an abreviation ::: src/nautilus-preferences-window.c @@ +362,3 @@ } +static void bind_builder_uint_spin(GtkBuilder *builder, spurious spaces before the parameter @@ +368,3 @@ +{ + g_settings_bind (settings, prefs, gtk_builder_get_object (builder, widget_name), + "value", G_SETTINGS_BIND_DEFAULT); one space missing before "value"
Created attachment 347544 [details] [review] preferences-window: Use spin button for thumbnail limit File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox in the preferences to a spinbutton and changes the signal handler for thumbnail-limit. The value is in MB, with a maximum of 4096 and a minimum of 1.
This change: - <property name="label" translatable="yes">Onl_y for files smaller than:</property> + <property name="label" translatable="yes">Onl_y for files smaller than :</property> is incorrect. Please don’t put a space before the colon.
Created attachment 347548 [details] [review] preferences-window: Use spin button for thumbnail limit File sizes (especially compressed images) are arbitrary values that are highly dependant on the camera resolution, and contents of the image. Therefore it does not make sense to have this be a drop-down with preset hard-coded values: A 21MP camera often produces 12MB images... but the only option is 10MB, or 100MB. This commit changes the combobox in the preferences to a spinbutton and changes the signal handler for thumbnail-limit. The value is in MB, with a maximum of 4096 and a minimum of 1.
Review of attachment 347548 [details] [review]: Sorry it took long, I was not understanding why video thumbnails are not inside this limit. This code needs a little clarification, but that's work for future! :) It looks good now, thanks for your work!
Attachment 347548 [details] pushed as 611f381 - preferences-window: Use spin button for thumbnail limit