search

enkore / j4-dmenu-desktop

A fast desktop menu
GNU General Public License v3.0
682 stars 69 forks source link

Improve j4-dmenu-desktop #132

Closed meator closed 1 year ago

meator commented 2 years ago

@enkore I've messaged you some time ago about my intention of contributing to j4-dmenu-desktop. This is what I've been working on.

Inotify (and kqueue) support!

This is the main new feature I've added. When you run j4dd in wait on mode, it will now automatically detect the installation and uninstallation of programs while j4dd is running. J4dd will then update the list of programs.

I had to completely overhaul Applications to make live updates to it possible. It's now much more complex but it's relatively fast because it uses hashing and it keeps track of iterators to desktop file data. This allows to skip shadowed desktop files (this is described later). It keeps track of programs with the same name - there will no longer be three identical "Web browser" entries which all launch the same program.

Programs with identical names

Because Applications now stores all desktop files with the same Name and GenericName, I have a proposal: when an user selects an ambiguous name, dmenu could be run for the second time with all desktop files which have that name.

If you would select "Web browser", there could be a second dmenu prompt that would allow you to pick from all the desktop entries with have the "Web browser" name (Firefox, Chromium, Opera...).

This would be relatively simple to implement because Applications already maintains a list of all desktop files which have the same name to avoid having multiple identical names in j4dd which all run the same program, so lookup of this information would be fast.

Issues

My first plan was to implement only a inotify backend and then make j4dd depend on libinotify-kqueue on systems which don't have inotify (FreeBSD). But libinotify-kqueue seems to be highly unreliable, it doesn't create any IN_MODIFY events on FreeBSD which makes it more complicated to use in j4dd. This would also add additional dependencies to j4dd on FreeBSD.

Because of this, I've made two backends: inotify and kqueue. CMake decides when to use which via the USE_KQUEUE option. It's set to YES on FreeBSD by default and to NO on other systems.

A better solution would be to use some library for this like fswatch. But I wasn't able to find a good C++/C library that would work on both Linux and FreeBSD. If you have any suggestions than please comment here.

I am not experienced with CMake. There are probably better ways to make this OS dependent system work but I've tested my modified CMakeLists.txt and it works well on both Linux and FreeBSD.

The inotify backend can detect the modification of a desktop file but the kqueue backend can't. This is due to technical limitations of kqueue. This is further described in NotifyKqueue.hh. The kqueue backend can still detect new and deleted desktop files.

Both backends currently don't handle the creation and deletion of entire directories in SearchPath. I didn't implement it yet because I don't think subdirectories in applications/ folders are often used. I have many desktop programs and only a single program made a subdirectory for its desktop files in /usr/share/applications (xfce screensavers). This could be implemented in the future.

The kqueue backend creates a separate thread that polls for kqueue changes. Because repeated polling could result in busy waiting, I've added a one minute sleep after a poll. This means that there could be up to one minute delay between desktop file addition or deletion and the detection of this event by NotifyKqueue. This significantly slows down unit tests on FreeBSD (but I don't think that is a problem).

Refactoring & bug fixing

The main focus of this PR is refactoring.

Classes

Some classes in j4dd don't really have to be classes. For example I don't see the reason why should SearchPath be a class. It exposes its iterators with begin() and end() which are tied to a internal list of searchpath directories. But if there is a internal list of entries and begin() and end() that just link to it, shouldn't it just return the "internal list"?

I see that this might have been done for encapsulation of the internal list, but I think it's reasonable to assume that SearchPath will return the directories in a stringlist_t. I've turned SearchPath into a simple function. There's less code now and it's now clear how the directories should be accessed. The old SearchPath defined only begin() and end(), but now when the actual list is returned, you have full access to the list.

Memory management

I successfully removed all new and delete calls from j4dd. Dynamic memory is now safely and automatically managed by containers.

RAII

The constructor should initialize the class. Application::Application(const LocaleSuffixes&, const stringlist_t*) does nothing. The "real" constructor of Application is Application::read(const char*, char**, size_t*). Dmenu::Dmenu(const std::string&) calls the "real" constructor, the Dmenu::create(). LocaleSuffixes::LocaleSuffixes() has exactly one line of code:

generate(locale);

I've moved all this stuff into the constructors. It's now clearer how are these classes supposed to be initialized. This means that error handling (primarily in Application in this PR) is now handled by exceptions.

Do not read things that don't have to be read

The old collect_files() reads SearchPath backwards. This ensures that directory entries are read in the right order and that when there is a collision in desktop file IDs, the desktop entry that appears earlier in $XDG_DATA_DIRS is chosen. But this means that j4dd will read and parse the entire desktop entry just to throw it away and overwrite it with the one that appears earlier in $XDG_DATA_DIRS. The new code just skips desktop entries that collide with the correct ones.

When Hidden or NoDisplay (or OnlyShowIn and NotShowIn when -x) disables a desktop file, the old code still finishes parsing the file. We can and should ignore a disabled desktop file the moment we discover it's disabled, because further parsing of the desktop file is useless. Application now throws disabled_error when this occurs and then Applications catches it and then just ignores the desktop file.

It is unlikely that there will be colliding desktop entries. These situations aren't very common. But changes I had to made to Applications to make Notify work allowed me to do improve this without adding much runtime cost.

Compliance with the spec

I've improved the compliance with the XDG spec in many areas, but the main one is escaping. The old implementation just calls replace() a bunch of times in ApplicationRunner and calls it a day. This applies only to the Exec key, other keys aren't escaped at all which is against the spec. Escaping is now handled properly (except %f, %F, %u and %U and some other things which are more complicated).

The --wrapper problem

The situation with escaping in --wrapper isn't ideal. The changes I made to ApplicationRunner made it even worse.

I don't fully understand what is the purpose of --wrapper. Please correct me if I'm wrong but I think the only purpose of --wrapper is to make --wrapper i3\ exec possible. It's pretty much impossible to escape this because i3 exec has special escape rules and it uses a double (!) backslash to escape things. We could handle this, but then --wrapper wouldn't be usable for other wrapper programs. Because I don't think --wrapper has much use apart from i3 exec, I propose (optional) i3 only integration with i3 IPC. This would replace the --wrapper option. This shouldn't be mandatory, I myself don't use i3 but it would allow correct i3 specific escaping. J4dd would then talk with i3 directly through IPC and not with i3 exec which has some advantages.

Terminal=true shell scripts

Why does ApplicationRunner create a temporary shell script to execute a terminal program? I have removed this because I don't know why is this done but if there is a reason for this, please comment on this PR and explain.

When a terminal program is encountered, j4dd will now execute <terminal emulator> -e <command string>.

I've noticed that a lot of characters that have to be quoted in the Exec key are related to the shell. With some help from people on the XDG mailing list, I've changed how escaping is done in ApplicationRunner. The shell specific characters must be quoted to allow Exec to be passed pretty much directly to the shell without caring about escaping ourselves. The shell will then will then expand the arguments by its rules which conveniently match the rules of Exec key expansion.

Some questions

While reading the code, I came across some things I didn't understand.

Another thing I noticed is that you can select multiple entries in dmenu with Ctrl-Return. J4dd currently only executes the first selected desktop entry. I didn't even know you can select multiple entries in dmenu until I read the manpage but this could be implemented so that j4dd would execute multiple programs.

Testing

I've tested these changes on Void Linux x86_64 glibc (my main system) and on FreeBSD 13.1-RELEASE (in a VM). My FreeBSD testing wasn't as extensive but NotifyKqueue should hopefully work there and I ran unit tests on FreeBSD and they succeeded.

If you have any questions or suggestions than please comment here. I'd like to continue working on j4dd in the future to implement some ideas I've mentioned here and to extend unit tests. There are many changes I've implemented but not mentioned in this message, this is just the summary of the changes.

English is not my mother tongue; please excuse any errors on my part.

meator commented 1 year ago

Hey @enkore, It's been a year since I sent a pull request to your repository, but I haven't received any feedback. Any chance you could take a look and let me know what you think? I put work into it and would appreciate your input.

enkore commented 1 year ago

Why is FileFinder using is_directory() to determine if a file is a directory when FileFinder is traversing the directory with readdir() which has d_type? Is a extra stat() syscall made because d_type isn't portable? I don't know any POSIX system which doesn't support d_type.

d_type does widely exist, but not all file systems actually store that in their dirents. Off the top of my head, XFS doesn't and will always give you DT_UNKNOWN.

Why exactly is setsid() being used here?

https://github.com/enkore/j4-dmenu-desktop/commit/a1f64710a0dbbb68ecc5076ea3f7e60c6450559b I'm guessing this became necessary with --wait-on

I don't know the answers to the other questions

enkore commented 1 year ago

I'd also like to apologize for not looking at this at all for a long time, I'm unfortunately quite horrible about tending to these projects (selfishly in part because I'm not using them any more). Your additions seem quite worthwhile, and since you appear to have a continued interest in the project I'd like to pass it on to you.

meator commented 1 year ago

Thanks for inviting me to contribute! I kind of lost my hope of working on j4-dmenu-desktop again. This has really surprised me!

This is probably my first open source project that I'm having a larger role in. I'll try to do my best to improve and maintain j4dd. You won't regret your decision :-)

About the technical side of things, I'm also a bit busy now. I'll fix up this PR next week.

meator commented 1 year ago

I have started to plan the r3.0 release. I think my changes & my maintainership justify a major version bump. I'm glad that I can push j4dd forward!

To react to your responses:

d_type does widely exist, but not all file systems actually store that in their dirents. Off the top of my head, XFS doesn't and will always give you DT_UNKNOWN.

I didn't know XFS does that. I will keep the stat() call then.

Why exactly is setsid() being used here?

I'm guessing this became necessary with --wait-on

I remember being confused by this when making this PR. I didn't believe sessions and process group leaders were relevant to j4dd. I have even straced XFCE's builtin menu program and found no call of setsid(). I will investigate this further and probably remove it after testing.

I will need to refamiliarize myself with j4dd's codebase which will take me some time. I will inspect this PR and look for errors. I remember once getting a segfault related to inotify but I didn't investigate it at that time so I would like to find the cause now. I'll also rebase develop to resolve merge conflict.

duarm commented 1 year ago

Just here to thank the efforts of both enkore and meator, good job guys, I use j4 everyday

meator commented 1 year ago

I have decided to merge this PR. I still don't consider this PR complete but it's more practical for me to work directly on enkore/j4-dmenu-desktop rather than on meator/j4-dmenu-desktop. This will prevent others from making PRs incompatible with this PR.

I will try to "refactor my refactor" (Applications.hh is pretty messy now and it's my fault) and complete everything in the r3.0 milestone. Mainly #144. I have "fixed" the merge conflict by reverting #137. This isn't really a fix, I have just deleted the conflicting code. I will reimplement this, but there are issues that have to be fixed first.

The develop branch shouldn't be considered stable for now. I don't think anyone is running j4dd from HEAD. Versioned releases should be trusted instead.

meator commented 4 months ago

Terminal=true shell scripts

Why does ApplicationRunner create a temporary shell script to execute a terminal program? I have removed this because I don't know why is this done but if there is a reason for this, please comment on this PR and explain.

When a terminal program is encountered, j4dd will now execute <terminal emulator> -e <command string>.

I have now realized why this is done. I will have to reintroduce this feature. In my defense, this feature could have been documented a bit better. There wasn't a single comment explaining this complicated feature.

My changes break terminal applications when using specific terminal emulators, primarily gnome-terminal. The change is in 575d05fe72491c0dacc476a7ca9f17817b68f98c. It will have to be undone.