Comment 1 alexxcons 2020-03-19 23:38:20 CET

Nice little features !

These suggestions are all fine for me.

First I thought it would be nice to have more consitency, by showing the path for all locations in the first line, and some additional info in the second line.
Though I guess that might be to much irrelevant text for the standard folders.

In the code, I would rename "device_string" to tooltip.

Another small thing:

              device_location = g_file_get_uri (file);
              device_string = g_strdup_printf ("%s", device_location);

              g_value_take_string (value, device_string);

              g_free (device_location);

Why not directly g_value_take_string (value, g_file_get_uri (file)); ?

Comment 2 Theo Linkspfeifer 2020-03-20 12:39:21 CET

Created attachment 9618 
diff

After looking at the code again I had to rework it a bit.

Comment 3 alexxcons 2020-03-20 22:47:11 CET

More readable now, and you found a better location for the tooltips of home/desktop/computer. Nice !

Something here looks fishy:

          if (G_LIKELY (file != NULL))
            {
              if (shortcut->device != NULL)
                {
                  shortcut->tooltip = g_file_get_uri (file);
                  g_object_unref (file);
                }
              else
                {
                  parse_name = g_file_get_parse_name (file);
                  shortcut->tooltip = g_markup_escape_text (parse_name, -1);
                  g_free (parse_name);
                }
            }

Before your patch, "file" was not freed at all, Now it is only is freed in the "if" case. If that is on purpose, a comment would be good.

Comment 4 Theo Linkspfeifer 2020-03-20 23:35:51 CET

These lines were added as replacement for the THUNAR_SHORTCUT_GROUP_NETWORK_MOUNTS block:

+          if (shortcut->device != NULL)
+            file = thunar_device_get_root (shortcut->device);

+              if (shortcut->device != NULL)
+                {
+                  shortcut->tooltip = g_file_get_uri (file);
+                  g_object_unref (file);
+                }

So, only thunar_device_get_root() requires the g_object_unref() call.

Comment 5 alexxcons 2020-03-23 09:37:15 CET

Oh, I see. In that case I would rather split the if/else structure in a different way:

          if (shortcut->device != NULL)
            {
              file = thunar_device_get_root (shortcut->device);
              if (G_LIKELY (file != NULL))
                {
                  shortcut->tooltip = g_file_get_uri (file);
                  g_object_unref (file);
                }
            }
          else
            {
              if (shortcut->file != NULL)
                file = thunar_file_get_file (shortcut->file);
              else if (shortcut->location != NULL)
                file = shortcut->location;
              else
                file = NULL;
              if (G_LIKELY (file != NULL))
                {
                  parse_name = g_file_get_parse_name (file);
                  shortcut->tooltip = g_markup_escape_text (parse_name, -1);
                  g_free (parse_name);
                }
            }

Comment 6 alexxcons 2020-03-23 09:41:09 CET

Created attachment 9631 
patch

Would be ok for you to push like this ?

Comment 7 Theo Linkspfeifer 2020-03-23 11:17:07 CET

Sure. Maybe readd this new line though:

          else
            file = NULL;

          if (G_LIKELY (file != NULL))

Comment 8 Git Bot 2020-03-23 22:35:30 CET

Theo Linkspfeifer referenced this bugreport in commit 791e785a7d23387c7313d1934250ef35be544625

Improve tooltips in shortcuts view (Bug #16566)

https://git.xfce.org/xfce/thunar/commit?id=791e785a7d23387c7313d1934250ef35be544625

Comment 9 alexxcons 2020-03-23 22:37:09 CET

(In reply to Theo Linkspfeifer from comment #7)
> Sure. Maybe readd this new line though:
> 
>           else
>             file = NULL;
> 
>           if (G_LIKELY (file != NULL))

Yes, that looks better, done!

Pushed it to master, thank you for your work !

Comment 10 Theo Linkspfeifer 2020-04-18 21:52:39 CEST

Created attachment 9746 
Show URI in tooltip for devices connected via MTP

...instead of the "Browse the file system" text which is only meant for file:/// .

Comment 11 Theo Linkspfeifer 2020-04-18 22:10:10 CEST

Created attachment 9747 
Hide disk usage in tooltip if not available

Android phone which was not mounted yet shows "(null)" for disk usage.

Comment 12 Git Bot 2020-04-19 10:44:25 CEST

Theo Linkspfeifer referenced this bugreport in commit 1d02123fc3017f2912dd5828c0780aac98f359aa

Show URI in tooltip for devices connected via MTP (Bug #16566)

https://git.xfce.org/xfce/thunar/commit?id=1d02123fc3017f2912dd5828c0780aac98f359aa

Comment 13 Git Bot 2020-04-19 10:44:28 CEST

Theo Linkspfeifer referenced this bugreport in commit 702c69f30b26b434c3f16fafc4030189b4c6bdeb

Hide disk usage in tooltip if not available (Bug #16566)

https://git.xfce.org/xfce/thunar/commit?id=702c69f30b26b434c3f16fafc4030189b4c6bdeb

Comment 14 alexxcons 2020-04-19 10:45:33 CEST

Thanks Theo!

Can reproduce the problem on both, tested patches, work both fine for me .. pushed both to master