Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] xdg thumbnails fetching with fallback on mimetype icons #1939

Open
wants to merge 34 commits into
base: next
Choose a base branch
from

Conversation

giomatfois62
Copy link

This implements xdg thumbnails fetching functionality in rofi-icon-fetcher.

When an icon is requested for an entry in filebrowser mode, and its name is prefixed by the string "thumbnail://", an existing thumbnail is searched in $HOME/.cache/thumbnails/ and loaded as icon for the entry.

If the thumbnail does not exists, an icon for the entry mime-type is loaded instead.

If the entry is a ".desktop" file, its icon key is used instead to load the icon.

WIP because it lacks the functionality to generate missing thumbnails using system thumbnailers and an option for users to use their own scripts as custom thumbnailers.

@DaveDavenport
Copy link
Collaborator

Thanks, looks good.. What is the license of the md5 code? can we include it?

@giomatfois62
Copy link
Author

giomatfois62 commented Jan 28, 2024

it seems very much free to use. from the LICENSE

This is free and unencumbered software released into the public domain.
Anyone is free to copy, modify, publish, use, compile, sell, or
distribute this software, either in source code form or as a compiled
binary, for any purpose, commercial or non-commercial, and by any
means.

Don't know if you prefer it as a 3rd party submodule, last commit was almost a year ago

@DaveDavenport
Copy link
Collaborator

Great. We should copy the license into the header of the file then, so it is clear how it is licensed.

Some things I noted trying it out:

  • Fix meson build
  • Should we be more agrassive in finding thumbnail? for me it fails as it looks in wrong directory.

I will try it out further.

Thanks!

@DaveDavenport
Copy link
Collaborator

rofi-2024-01-28-1825-00000

Seems to work!

@giomatfois62
Copy link
Author

Sorry I completely forgot about meson, I tend to use configure.
As for the missing thumbnails, i look for them in the folder returned by g_get_user_cache_dir, on my system it points to $HOME/.cache

@DaveDavenport
Copy link
Collaborator

That is not what I tried to point out.. for me it looks for thumbnail that does exists, but only larger.

I worked around this as a test like this:

 // build file thumbnail's path using md5 hash of its encoded uri string
-static gchar* rofi_icon_fetcher_get_file_thumbnail(const gchar* file_path, 
-                                                   int requested_size, 
+static gchar *rofi_icon_fetcher_get_file_thumbnail(const gchar *file_path,
+                                                   int requested_size,
                                                    int *thumb_size) {
   // convert filename to encoded uri string and calc its md5 hash
   gchar *encoded_entry_name = g_filename_to_uri(file_path, NULL, NULL);
@@ -319,49 +319,71 @@ static gchar* rofi_icon_fetcher_get_file_thumbnail(const gchar* file_path,
   md5_to_hex(md5_hex, hex_size, md5, md5_size);
 
   // determine thumbnail folder based on the request size
-  const gchar* cache_dir = g_get_user_cache_dir();
-  gchar* thumb_path;
+  const gchar *cache_dir = g_get_user_cache_dir();
+  gchar *thumb_path = NULL;
 
   if (requested_size <= 128) {
     *thumb_size = 128;
-    thumb_path = g_strconcat(cache_dir, "/thumbnails/normal/",
-        md5_hex, ".png", NULL);
-  } else if (requested_size <= 256) {
+    thumb_path =
+        g_strconcat(cache_dir, "/thumbnails/normal/", md5_hex, ".png", NULL);
+  }
+  if (thumb_path && g_file_test(thumb_path, G_FILE_TEST_EXISTS)) {
+    return thumb_path;
+  }
+  g_free(thumb_path);
+  thumb_path = NULL;
+
+  if (requested_size <= 256) {
     *thumb_size = 256;
-    thumb_path = g_strconcat(cache_dir, "/thumbnails/large/",
-        md5_hex, ".png", NULL);
-  } else if (requested_size <= 512) {
+    thumb_path =
+        g_strconcat(cache_dir, "/thumbnails/large/", md5_hex, ".png", NULL);
+  }
+
+  if (thumb_path && g_file_test(thumb_path, G_FILE_TEST_EXISTS)) {
+    return thumb_path;
+  }
+  g_free(thumb_path);
+  thumb_path = NULL;
+
+  if (requested_size <= 512) {
     *thumb_size = 512;
-    thumb_path = g_strconcat(cache_dir, "/thumbnails/x-large/",
-        md5_hex, ".png", NULL);
-  } else {
+    thumb_path =
+        g_strconcat(cache_dir, "/thumbnails/x-large/", md5_hex, ".png", NULL);
+  }
+  if (thumb_path && g_file_test(thumb_path, G_FILE_TEST_EXISTS)) {
+    return thumb_path;
+  }
+  g_free(thumb_path);
+  thumb_path = NULL;
+
+  {
     *thumb_size = 1024;
-    thumb_path = g_strconcat(cache_dir, "/thumbnails/xx-large/",
-        md5_hex, ".png", NULL);
+    thumb_path =
+        g_strconcat(cache_dir, "/thumbnails/xx-large/", md5_hex, ".png", NULL);
   }
 
   return thumb_path;
 }

I did something like this to make it find more thumbnails.

@DaveDavenport
Copy link
Collaborator

This is not ideal, I think we should reverse it.. find the biggest one exists (we will auto-scale it down) and work our way down..

Because now I don't see it in a theme with bigger 'icons'. (I only have thumbs in normal folder).

Is there a nice way to trigger the generation of thumbnails? are these script shipped?

@giomatfois62
Copy link
Author

Is there a nice way to trigger the generation of thumbnails? are these script shipped?

That's the next step, it's still a work in progress. There is a specification for thumbnailers that is used by file managers. They are ".desktop" files placed in /usr/share/thumbnailers" and "$HOME/.local/share/thumbnailers", containing a list of compatible mime-types and the corresponding to command to generate their thumbnails.
The command takes some arguments coded as follows:

  • %i is replaced by the filename
  • %u is replaced by an encoded uri of the filename
  • %o is the output file path of the thumbnail
  • %s is the requested size (max between width and height)

I'm looking at the implementation in libgnome-desktop and was thinking of using a similar solution.

Create an hashmap of mimetypes as keys and compatible thumbnailers as values. This can be placed in the global state of rofi-icon-fetcher-data, and populated at creation reading from the thumbnailers standard directories.
When a thumbnail is not found in the cache, look its mimetype in the hashmap and call the corresponding thumbnailer command expanding the arguments, we have already filename, encoded uri and target size.

Doing this will make thumbnails generated by rofi compatible with other file managers

@giomatfois62
Copy link
Author

Seems to work pretty good now, I suggest you to first install the xapp-thumbnailers, this way you have thumbnailers available for epub, mp3, raw files and others.
rofi-2024-01-30-2101-00000
rofi-2024-01-30-2105-00000
rofi-2024-01-30-2106-00000
rofi-2024-01-30-2110-00000
rofi-2024-01-30-2112-00000

@DaveDavenport
Copy link
Collaborator

that looks very nice.

@giomatfois62
Copy link
Author

to complete this work, i would like to add the ability to specify a custom thumbnailer command when launching rofi.
I was thinking something like a "-preview-cmd" or "-icon-fetcher" key that takes a string with the command or path to the script to call. If that is specified, we can call it instead of the standard thumbnailers for mimetypes.

We could keep saving the thumbnails in the xdg cache directories, the custom script would take as arguments the entry name, the output path of the thumbnail (generated with the md5 hash of the entry name) and an integer size. Then, in the script one could do cool things based on the size requested, like for a list of games fetching their icons for small previews and high res banners for bigger previews.

A practical example would be something like this:

rofi -show-icons -preview-cmd "/path/to/custom/script %i %o %s"

%i, %o and %s will then be substituted by entry name, ouptut image path and requested size in rofi-icon-fetcher, the code to do that is already there to use standard thumbnailers.

What do you think?

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Jan 31, 2024

sounds good.. maybe look at available helper functions.. There is already code to launch applications and do substitution.
(different format, using {} like {name} )

https://github.com/davatorium/rofi/blob/next/source/modes/ssh.c#L97 example ( need to check, this leaks memory in argsv?)

@giomatfois62
Copy link
Author

You mean helper_parse_setup and helper_execute?
I looked at helper_execute before, but we need to wait for the thumbnailer processes to finish, so I used g_spawn_sync instead.

@DaveDavenport
Copy link
Collaborator

helper_parse_setup mostly, as it does the replacement.

@giomatfois62
Copy link
Author

helper_parse_setup mostly, as it does the replacement.

I see, it does exactly what I need. Thanks for the pointer!

@giomatfois62
Copy link
Author

I tried using helper_parse_setup to build to list of arguments of the thumbnailers, but I'm facing an issue. I need to replace the strings "%i", "%o", "%s" but the function is leaving them unchanged.
This is my call:
helper_parse_setup(command, &command_args, &argsv, "{%i}", filename, "%u", encoded_uri, "%o", output_path, "%s", size_str, (char *)0);
I tried using some encodings like "%%i", "%i", "\%i" and others to no avail.

@DaveDavenport
Copy link
Collaborator

It does not support replacing '%i'.. it replaces anything between brackets..

so ssh {host} it replaces {host} with the actual hostname.
In the above example I would make the keywords '{filename}' '{uri}' '{output}' or something similar.

@giomatfois62
Copy link
Author

giomatfois62 commented Feb 2, 2024

okay thanks, now i understand, we can use it for the custom thumbnailer scripts but not for the xdg thumbnailers installed in the system.
Then I'll use it to better conform with the rest of rofi custom command options.

I was testing performance using the new recursive browser mode, and I got a little worried about performance, cpu is trashed and scrolling becomes very very slow after some pages.
I want to check if there is some way to set process niceness to make the thumbnailers more resource friendly.

UPDATE: solved prepending "nice -n 19" to the command arguments list, much much better performance

@DaveDavenport
Copy link
Collaborator

In theory it should be 'async' from the UI, but trashing your I/O it often becomes more difficult for the scheduler to do a good job..

I hope to be able to play with the PR this weekend.

Thanks.

@giomatfois62
Copy link
Author

giomatfois62 commented Feb 2, 2024

I pushed the latest changes, it now works for me also using a custom script. To test, run something like this:
echo ... | rofi -dmenu -show-icons -preview-cmd "/path/to/custom/script {input} {output} {size}"
and put something in the echo to set the icon, like
"A\x00icon\x1fthumbnail://A\nB\x00icon\x1fthumbnail://B\nC\x00icon\x1fthumbnail://C"

@DaveDavenport
Copy link
Collaborator

took me a while to get it working as on debian apparmour was blocking evince-thumbnailer from writing files when launched by rofi..
(we should include this in documentation)..

It works really well, I will play with it some more later..

I did notice that for some reason fallback icon did not work anymore, but did not get around debugging this.

@giomatfois62
Copy link
Author

took me a while to get it working as on debian apparmour was blocking evince-thumbnailer from writing files when launched by rofi.. (we should include this in documentation)..

I had some problems too, some commands were not recognized despite being in /usr/bin, I had to set it as the working directory of the spawned process to make it work. That's not very clean as a workaround to be honest.

I did notice that for some reason fallback icon did not work anymore, but did not get around debugging this.

That's strange, i see them in filebrowser mode, there was something broken with the last "custom command" addition but it should be fixed with the last commit, I did not touch anything about mimetype icons fallback.

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Feb 5, 2024

../source/rofi-icon-fetcher.c:167:3: error: unknown type name ‘GStrvBuilder’
167 | GStrvBuilder *cmd_builder = g_strv_builder_new();

I think we need to bump the glib dependency to use this method.

https://docs.gtk.org/glib/ctor.StrvBuilder.new.html

2.68 so that excludes ubuntu 20.04 LTS

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Mar 7, 2024

static void
g_thread_pool_free_internal (GRealThreadPool* pool)
{
  g_return_if_fail (pool);
  g_return_if_fail (pool->running == FALSE);
  g_return_if_fail (pool->num_threads == 0);

  /* Ensure the dummy item pushed on by g_thread_pool_wakeup_and_stop_all() is
   * removed, before it’s potentially passed to the user-provided
   * @item_free_func. */
  g_async_queue_remove (pool->queue, GUINT_TO_POINTER (1));

  g_async_queue_unref (pool->queue);
  g_cond_clear (&pool->cond);

  g_free (pool);
}

well.. ok.. But I think I am catching the 1 passed here.

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Mar 7, 2024

Ok. the above fix was added in 2.79.. and I am running older

GNOME/glib@b402f66

@DaveDavenport
Copy link
Collaborator

diff --git a/source/rofi-icon-fetcher.c b/source/rofi-icon-fetcher.c
index a6ddbcb0..99620ce0 100644
--- a/source/rofi-icon-fetcher.c
+++ b/source/rofi-icon-fetcher.c
@@ -246,8 +246,9 @@ static gboolean rofi_icon_fetcher_create_thumbnail(const gchar *mime_type,
 }
 
 static void rofi_icon_fetch_thread_pool_entry_remove(gpointer data) {
-  IconFetcherNameEntry *entry = (IconFetcherNameEntry *)data;
+  IconFetcherEntry *entry = (IconFetcherEntry *)data;
   // Mark it in a way it should be re-fetched on next query?
+  entry->query_started = FALSE;
 }
 
 static void rofi_icon_fetch_entry_free(gpointer data) {
@@ -535,8 +536,6 @@ static void rofi_icon_fetcher_worker(thread_state *sdata,
   const gchar *icon_path;
   gchar *icon_path_ = NULL;
 
-  sentry->query_started = TRUE;
-
   if (g_str_has_prefix(sentry->entry->name, "thumbnail://")) {
     // remove uri thumbnail prefix from entry name
     gchar *entry_name = &sentry->entry->name[12];
@@ -761,7 +760,7 @@ uint32_t rofi_icon_fetcher_query_advanced(const char *name, const int wsize,
   sentry->hsize = hsize;
   sentry->entry = entry;
   sentry->query_done = FALSE;
-  sentry->query_started = FALSE;
+  sentry->query_started = TRUE;
   sentry->surface = NULL;
 
   entry->sizes = g_list_prepend(entry->sizes, sentry);
@@ -804,7 +803,7 @@ uint32_t rofi_icon_fetcher_query(const char *name, const int size) {
   sentry->hsize = size;
   sentry->entry = entry;
   sentry->query_done = FALSE;
-  sentry->query_started = FALSE;
+  sentry->query_started = TRUE;
   sentry->surface = NULL;
 
   entry->sizes = g_list_prepend(entry->sizes, sentry);
diff --git a/source/view.c b/source/view.c
index 15c72865..4965c306 100644
--- a/source/view.c
+++ b/source/view.c
@@ -2652,7 +2652,7 @@ static int rofi_thread_workers_sort(gconstpointer a, gconstpointer b,
 }
 
 static void rofi_thread_pool_state_free(gpointer data) {
-  if (data) {
+  if (data && GPOINTER_TO_UINT(data) != 1) {
     thread_state *ts = (thread_state *)data;
     if (ts->free) {
       ts->free(data);

This seems to fix it for me.

There is one change I would like to make, I do not like that listview calls the destroy/create method from view.c on the thread pool.
This breaks the abstraction that is there..

Maybe for now lets make a callback on the listview for a page change and then call the destroy/create inside view.c.

@giomatfois62
Copy link
Author

XD the rogue dummy item!! Great work Dave, I'll update the PR as soon as I get some free time

@DaveDavenport
Copy link
Collaborator

pushed fix to next branch.

@giomatfois62
Copy link
Author

I added the callback on the listview for page changes, all working well in my tests

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Mar 9, 2024

I guess it is time to merge this. @lbonn should we merge this for next release or lift over the release and try to do a quick follow up release.

@lbonn
Copy link
Collaborator

lbonn commented Mar 9, 2024

@DaveDavenport hm good question... In the notes, the filebrowser is presented as experimental so probably it is fine to include it in the release as such.

It would just need some light documentation I suppose.

@giomatfois62
Copy link
Author

I can add documentation too, where should I put it? My guess was in rofi-theme

@DaveDavenport
Copy link
Collaborator

I think this is a good location: https://github.com/davatorium/rofi/blob/next/doc/rofi-theme.5.markdown#icon-handling
To add some information. If it needs a large explanation (to explain setup, etc), we might want to have a separate manpage.

I am still wondering if we should make this the 'default' behaviour for loading images, so we re-use preview other programs make.. The thing that worries me a little is that I had to tweak app-armour on my pcs to make it work.

@giomatfois62
Copy link
Author

I can't help much with that at the moment, I use Slackware and it doesnt come with neither apparmor nor selinux. I can test a bit at work with fedora/Ubuntu in the coming days if you can wait a bit for the new release

@DaveDavenport
Copy link
Collaborator

No rush, I am crazy busy too.
I will test it too on ubuntu/debian and can update the document.

@giomatfois62
Copy link
Author

I'm investigating the apparmor permission issues. Looking at the gnome code here apparently the thumbnailers programs (the one specified in the Exec key of .thumbnailer files) are not trusted by apparmor/selinux/flatpak etc. and are restricted to write inside a sandbox, that is a temporary folder created in /tmp/. The generated images are then loaded back in memory and saved in the standard thumbnails paths ($HOME/.cache/thumbnails/...).
Now I want to study how xfce does it (with their tumblerd implementation), kde does everything by itself using dolphin plugins and doesnt look for .thumbnailer files in the system like we do

@giomatfois62
Copy link
Author

can you try adding the following apparmor profile to /etc/apparmor.d/usr.bin.rofi

#vim:syntax=apparmor
# AppArmor policy for rofi

#include <tunables/global>

/usr/bin/rofi {
    #include <abstractions/base>

    # TCP/UDP network access for NFS
    network inet  stream,
    network inet6 stream,
    network inet  dgram,
    network inet6 dgram,

    /usr/bin/rofi mr,

    @{HOME}/ r,
    @{HOME}/** rw,
    owner @{HOME}/.cache/thumbnails/** rw,
}

then run apparmor_parser -r /etc/apparmor.d/usr.bin.rofi to reload the rule. This assumes that rofi binary is in /usr/bin.
I adapted the profile of evince-thumbnailer together with a snippet taken from here

@DaveDavenport
Copy link
Collaborator

I will test this somewhere this week.. Thanks a lot

@DaveDavenport
Copy link
Collaborator

did not forget, will get back to testing.

@DaveDavenport
Copy link
Collaborator

I think one of the important things left todo is test the above, but mostly document this feature.
Anybody willing to take this task?

@giomatfois62
Copy link
Author

I can write some documentation, perhaps also pointing to the apparmor rule if it works for everyone (I only tested it on my work pc), where do you think is best to put it?

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Apr 26, 2024

Currently we have a small section here on icon loading: https://github.com/davatorium/rofi/blob/next/doc/rofi-theme.5.markdown#icon-handling

However it might be good to create a new manpage for this, given the questions we get it could not hurt to expand the current section and add the new info..

@lbonn what do you think?

@lbonn
Copy link
Collaborator

lbonn commented Apr 26, 2024

I agree, it is probably worthy of its own man page, with a link (and adjustments) in the existing section.

rofi-filebrowser.5 or rofi-thumbnails.5? If we pick filebrowser, we also have a space to document the mode(s) with a little bit more details.

@DaveDavenport
Copy link
Collaborator

I would go for rofi-thumbnails, because I think it would be nice to (in a next version) use this more generic .

@giomatfois62
Copy link
Author

I finally got some time to write documentation of the new feature, let me know what you think about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants