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

Menubar loads desktop files incorrectly #1816

Open
nenadalm opened this issue Jun 4, 2017 · 10 comments · May be fixed by #3822
Open

Menubar loads desktop files incorrectly #1816

nenadalm opened this issue Jun 4, 2017 · 10 comments · May be fixed by #3822

Comments

@nenadalm
Copy link

nenadalm commented Jun 4, 2017

Output of awesome --version:

awesome v3.5.9 (Mighty Ravendark)
• Build: Mar 7 2016 18:22:52 for x86_64 by gcc version 6.0.0 (mockbuild@)
• Compiled against Lua 5.3.2 (running with Lua 5.3)
• D-Bus support: ✔

awesome v4.0 (Harder, Better, Faster, Stronger)
• Compiled against Lua 5.1.5 (running with Lua 5.1)
• D-Bus support: ✔
• execinfo support: ✔
• RandR 1.5 support: ✘
• LGI version: 0.9.0

How to reproduce the issue:
assuming following config:

menubar.menu_gen.all_menu_dirs = { "/usr/share/applications", "/usr/local/share/applications" }
  • install application (e.g. google-chrome)
  • open menubar (super+p)
  • start writing google - there is one entry 👍
  • $ cp /usr/share/applications/google-chrome.desktop /usr/local/share/applications/
  • restart awesome
  • open menubar (super+p)
  • start writing google - there are two entries 👎

If multiple files have the same desktop file ID, the first one in the $XDG_DATA_DIRS precedence order is used.

(https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#extra-actions-implementation-notes)

@vilev86
Copy link

vilev86 commented Feb 1, 2018

@fellowseb
Copy link

Hi,

I was wondering what you guys thought about making awesomewm's menubar follow the Desktop Menu Spec (https://specifications.freedesktop.org/menu-spec/latest/index.html) ?
I would like to give it a try (propose a PR) if nobody has an objection.

The idea would be to use .menu and .directory files instead of having the categories hard coded in a .lua script. It would be easier to reuse a same menu description between DEs and WMs.
Plus it would make us get rid of this bug in particular.

@Elv13
Copy link
Member

Elv13 commented Sep 14, 2018

@fellowseb All for it! Please also note that there is a couple of interesting pull requests open to improve some aspects such as the performance. Someone who would tie all the work together, create some unit tests and ensure compliance with the spec would do a great service to the AwesomeWM community. Another little thing I would do is move this into gears.desktopmenu or something like this so it isn't tied with the menubar library anymore. This way Debian can drop their package patches that add a very old code to add the menu to the top-left. Oh, and as always, if parts of it can be async, I would be very happy, but that's asking too much.

@psychon
Copy link
Member

psychon commented Sep 16, 2018

Note that we already have the code that follows the desktop menu spec. It is menubar.icon_theme and was deprecated in 0b0b466 because for some reason that no one understands, this was only used to look up the icons for categories and yet, just these few lookups made awesome come to a crawling halt.

The problem is not to implement the spec, but to implement the spec in a clever way, because all those file system lookups are too slow (see the commit message of 0b0b466).

I did some baby steps to fix our naive implementation.

Since the existing menubar.icon_theme cannot easily be changed to make use of these, nothing more happened.

@psychon
Copy link
Member

psychon commented Sep 16, 2018

Wait... I think I confused specs here. You were talking about .desktop files and not icon themes. Sorry! But now I am confused about what you actually want to do here... What are .menu and .directory files and where do they come from? On my current system, I only have google-chrome.menu and not a single .directory file in /usr/.

@fellowseb
Copy link

Thanks for the details @Elv13 !

I am indeed talking about the Menu Spec (and the associated Desktop Entry Spec). I am bringing this up because I too stumbled upon this same bug and I thought it would be a good occasion to work towards a complete compliance.
The spec uses .menu XML files to describe the menu itself, to reference the directories to be used when searching for the .desktop files and .directory files to categorise the programs in the menu. It allows users to easily override default menus and entries by adding files in their ~/.local/share/. This also means it would be easier for a user to keep consistent menus between different DEs/WMs.

In awesomewm, I see two use cases where this spec would make sense: the menubar and awful.menus (in particular, the mymainmenu instance in the default version of awesomerc.lua)

Here are a few distinct steps I would follow:

  • Create gears.desktop_menu (and maybe gears.desktop_entry), add lots of unit tests and use async computation when possible.
  • Make menubar call the previously created utilities to construct its data structure, without modifying its behavior and interface. Create a default /usr/local/share/menus/awesome-applications.menu to replace the current static Categories table.
  • The mymainmenu case: Add a way to construct a awful.menu (or a part of its items) from the previously created utilities. I think it's achievable to have the entire menu translated into .menu/.directory/.desktop files by using DBus calls to replace the awesome-related entries; but I'm not sure how valuable that would be (We could keep part of the menu coded in the lua file and just let people add items from .desktop/.menu/.directory files).

Your comments relative to the Icon Theme Spec are interesting, but that's another subject :).

Let me know if I'm totally wrong about all this; I don't want to hijack this thread! It would be my first contribution in Lua.

@actionless
Copy link
Member

btw here is a short example how to use menubar internals to generate an awful.menu: https://github.com/actionless/awesome_config/blob/devel/actionless/util/menugen.lua

@Elv13
Copy link
Member

Elv13 commented Sep 18, 2018

and btw one using the async APIs https://github.com/Elv13/awesome-configs/blob/master/customMenu/appmenu.lua

@psychon
Copy link
Member

psychon commented Sep 25, 2018

Quick & untested hack that might or might not work and that might or might not encourage someone to work on this properly:

diff --git a/lib/menubar/menu_gen.lua b/lib/menubar/menu_gen.lua
index 0be0b696..a40dd97d 100644
--- a/lib/menubar/menu_gen.lua
+++ b/lib/menubar/menu_gen.lua
@@ -10,6 +10,7 @@
 local gtable = require("gears.table")
 local gfilesystem = require("gears.filesystem")
 local utils = require("menubar.utils")
+local gio = require("lgi").Gio
 local pairs = pairs
 local ipairs = ipairs
 local table = table
@@ -82,17 +83,21 @@ function menu_gen.generate(callback)
     menu_gen.lookup_category_icons()
 
     local result = {}
-    local unique_entries = {}
+    local desktop_file_ids = {}
     local dirs_parsed = 0
 
+    -- TODO: Turn this into a protected call so that it does not crash awesome, similar to what is done in in utils.parse_dir
+    gio.Async.start(function()
+        -- TODO: Fix the indentation, but I wanted to keep the diff small
     for _, dir in ipairs(menu_gen.all_menu_dirs) do
-        utils.parse_dir(dir, function(entries)
+        utils.async_parse_dir(dir, function(entries)
             entries = entries or {}
             for _, entry in ipairs(entries) do
                 -- Check whether to include program in the menu
                 if entry.show and entry.Name and entry.cmdline then
-                    local unique_key = entry.Name .. '\0' .. entry.cmdline
-                    if not unique_entries[unique_key] then
+                    if not desktop_file_ids[entry.desktop_file_id] then
+                        desktop_file_ids[entry.desktop_file_id] = true
+
                         local target_category = nil
                         -- Check if the program falls into at least one of the
                         -- usable categories. Set target_category to be the id
@@ -115,7 +120,6 @@ function menu_gen.generate(callback)
                                      cmdline = cmdline,
                                      icon = icon,
                                      category = target_category })
-                        unique_entries[unique_key] = true
                     end
                 end
             end
@@ -125,6 +129,7 @@ function menu_gen.generate(callback)
             end
         end)
     end
+end)
 end
 
 return menu_gen
diff --git a/lib/menubar/utils.lua b/lib/menubar/utils.lua
index d0b9e5e0..1a819be9 100644
--- a/lib/menubar/utils.lua
+++ b/lib/menubar/utils.lua
@@ -407,12 +407,12 @@ function utils.parse_desktop_file(file)
     return program
 end
 
---- Parse a directory with .desktop files recursively.
+--- Parse a directory with .desktop files recursively in an LGI asynchronous context.
 -- @tparam string dir_path The directory path.
 -- @tparam function callback Will be fired when all the files were parsed
 -- with the resulting list of menu entries as argument.
 -- @tparam table callback.programs Paths of found .desktop files.
-function utils.parse_dir(dir_path, callback)
+function utils.async_parse_dir(dir_path, callback)
 
     local function get_readable_path(file)
         return file:get_path() or file:get_uri()
@@ -443,6 +443,7 @@ function utils.parse_dir(dir_path, callback)
                         if not success then
                             gdebug.print_error("Error while reading '" .. path .. "': " .. program)
                         elseif program then
+                            program.desktop_file_id = make_relative_path(dir_path, path)
                             table.insert(programs, program)
                         end
                     end
@@ -457,11 +458,18 @@ function utils.parse_dir(dir_path, callback)
         enum:async_close()
     end
 
-    gio.Async.start(do_protected_call)(function()
-        local result = {}
-        parser(gio.File.new_for_path(dir_path), result)
-        call_callback(callback, result)
-    end)
+    local result = {}
+    parser(gio.File.new_for_path(dir_path), result)
+    call_callback(callback, result)
+end
+
+--- Parse a directory with .desktop files recursively.
+-- @tparam string dir_path The directory path.
+-- @tparam function callback Will be fired when all the files were parsed
+-- with the resulting list of menu entries as argument.
+-- @tparam table callback.programs Paths of found .desktop files.
+function utils.parse_dir(dir_path, callback)
+    gio.Async.start(do_protected_call)(utils.async_parse_dir, dir_path, callback)
 end
 
 function utils.compute_textbox_width(textbox, s)

@dyfrgi
Copy link

dyfrgi commented Jun 14, 2023

Thanks for the starting point, @psychon. I used it as the basis for #3822, which fixes this.

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

Successfully merging a pull request may close this issue.

7 participants