Improve j4-dmenu-desktop #132
Conversation
meator
force-pushed
the
develop
branch
2 times, most recently
from
98346d5
to
02cfa86
Compare
|
Hey @enkore, |
| { | ||
| if (!p.second) { | ||
| fprintf(stderr, "Tried to insert an element when it was already present, aborting...\n"); | ||
| abort(); |
There was a problem hiding this comment.
Why not throw an exception instead?
There was a problem hiding this comment.
I am unsure because I haven't read the code thoroughly yet but it looks like the condition is fatal enough that there is no recovery possible and catching the exception wouldn't make sense. I choose abort() in code that I write if some internal invariant is broken.
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.
a1f6471 I'm guessing this became necessary with --wait-on I don't know the answers to the other questions |
|
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. |
|
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. |
|
I have started to plan the To react to your responses:
I didn't know XFS does that. I will keep the
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 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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This reverts commit c30fed9. This will be reimplemented to adapt to the new code.
main.cc contains the main() function. It should contain actual code that is related to running j4dd.
This is done to make functions usable without declarating functions they depend on beforehand.
Use local instead of global arguments and move buf to collect_files().
And add spaces around command.
std::string args will always be "" before apps.search() is called.
The standard specifies that the escape sequences \s, \n, \t, \r and \\ shall be recognised for the values of types string, localestring and iconstring and that the escape sequence \; (including the other ones) shall be recognised for values of type string(s). The previous implementation copies the values verbatim and therefore is nonconforming to the standard.
This commit changes the escaping logic because the previous commit changed how string values are expanded in Appplication. The escaping is now done by the shell like Qt/KDE does it. The field codes are now being escaped too to better comply with the standard. This commit completely removes the temporary script creation for Terminal=true programs. do_dmenu() now passes the program directly to the terminal emulator.
This makes the tests compile with the new changes implemented.
It is enabled by default, so there's no need to enable it.
This is related to d2ffc46
This makes the --wait-on mode more usable as a daemon. J4dd can now react to package installs and removals (which cause file modification events in the SearchPath directories) while j4dd is running.
Dmenu tries to waitpid() in Dmenu::read_choice() but zombies are already handled by the SIGCHLD signal handler when not in wait on mode. The signal handler is now established only when the wait on mode is active.
As nothed in the signal(2) manpage, the use of signal() should be avoided unless it's used for setting the disposition to SIG_DFL or SIG_IGN;
|
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" ( The |
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 |
@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
Applicationsto 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
Applicationsnow stores all desktop files with the sameNameandGenericName, 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
Applicationsalready 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_MODIFYevents 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_KQUEUEoption. 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.txtand 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 inapplications/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
SearchPathbe a class. It exposes its iterators withbegin()andend()which are tied to a internal list of searchpath directories. But if there is a internal list of entries andbegin()andend()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
SearchPathwill return the directories in astringlist_t. I've turnedSearchPathinto a simple function. There's less code now and it's now clear how the directories should be accessed. The oldSearchPathdefined onlybegin()andend(), but now when the actual list is returned, you have full access to the list.Memory management
I successfully removed all
newanddeletecalls 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 ofApplicationisApplication::read(const char*, char**, size_t*).Dmenu::Dmenu(const std::string&)calls the "real" constructor, theDmenu::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
Applicationin this PR) is now handled by exceptions.Do not read things that don't have to be read
The old
collect_files()readsSearchPathbackwards. 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_DIRSis 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
HiddenorNoDisplay(orOnlyShowInandNotShowInwhen-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.Applicationnow throwsdisabled_errorwhen this occurs and thenApplicationscatches 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
Applicationsto makeNotifywork 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 inApplicationRunnerand calls it a day. This applies only to theExeckey, other keys aren't escaped at all which is against the spec. Escaping is now handled properly (except%f,%F,%uand%Uand some other things which are more complicated).The --wrapper problem
The situation with escaping in
--wrapperisn't ideal. The changes I made toApplicationRunnermade 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--wrapperis to make--wrapper i3\ execpossible. It's pretty much impossible to escape this becausei3 exechas special escape rules and it uses a double (!) backslash to escape things. We could handle this, but then--wrapperwouldn't be usable for other wrapper programs. Because I don't think--wrapperhas much use apart fromi3 exec, I propose (optional) i3 only integration with i3 IPC. This would replace the--wrapperoption. 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 withi3 execwhich has some advantages.Terminal=true shell scripts
Why does
ApplicationRunnercreate 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
Execkey are related to the shell. With some help from people on the XDG mailing list, I've changed how escaping is done inApplicationRunner. The shell specific characters must be quoted to allowExecto 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 ofExeckey expansion.Some questions
While reading the code, I came across some things I didn't understand.
FileFinderusingis_directory()to determine if a file is a directory whenFileFinderis traversing the directory withreaddir()which hasd_type? Is a extrastat()syscall made becaused_typeisn't portable? I don't know any POSIX system which doesn't supportd_type.setsid()being used here?FileFinderuse a postfix increment operator to increment itself? It doesn't return the previous state. I am assuming this is a bug and I've changed it to a prefix increment operator.static const char *shellstatic?O_RDWR?GenericNameformatted withFormatterslikeNameis?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
NotifyKqueueshould 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.