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

Refactor the linux application menu #897

Merged
merged 11 commits into from Apr 15, 2017

Conversation

Projects
None yet
2 participants
@Stebalien
Contributor

Stebalien commented Feb 21, 2017

So, I've put this all into one PR for now so you can see it all at once but I'd be happy to break it into one PR per commit.

For details of the changes, please read the individual commit messages.

To Discuss:

  • Caching: I removed caching the application list because properly invalidating the cache is complicated and I didn't notice any slowdown (the menu is instantaneous). Added back with invalidation (the invalidation scheme could probably be made faster but I'm being conservative).

  • Open with prompt (counsel-linux-app-action-file): I removed the application "name" from this prompt because:

    • It was the first word of the Exec field which may not be meaningful (it's env in desktop files generated by WINE).

    • Given that I got rid of the application cache, I'd have to plumb through this information somehow. It's not hard, I'd just like to discuss what to do here before acting.

(match-string 1 (car entry))))
(file (and short-name
(read-file-name
(format "Run %s on: " short-name)))))

This comment has been minimized.

@Stebalien

Stebalien Feb 21, 2017

Contributor

This is the prompt I changed (the one where I removed the "name").

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Feb 21, 2017

Thanks. I agree with most of the changes. Some remarks:

  1. I'd like to keep counsel-linux-apps-faulty. It's a nice way for me to debug which desktop files aren't parsed properly. It also allows the user to manually fix these desktop files, so that counsel-linux-app works with them. Or to open bug report with a faulty desktop file attached.

  2. The removal of caching seems fine on one of my computers. The caching really makes sense to me though, because I restart Emacs more often (every few days or so) than installing new apps (every few months or so). But if the overhead is so small, let's continue without caching for now.

  3. You might want to look into the complexity of cl-remove-duplicates vs. delete-dups. The first seems to be O(N*N), while the second is O(N). I get only 146 entries on my system, so it's not a big deal for me, but it might be for someone else.

@Stebalien

This comment has been minimized.

Contributor

Stebalien commented Feb 21, 2017

I'd like to keep counsel-linux-apps-faulty.

Sure. I'll also add ones that fail to execute for some reason. Would you like me to avoid re-parsing faulty desktop files until the list is cleared?

You might want to look into the complexity of cl-remove-duplicates vs. delete-dups. The first seems to be O(N*N), while the second is O(N). I get only 146 entries on my system, so it's not a big deal for me, but it might be for someone else.

I've re-implemented it useing a hash table directly (delete-dups doesn't support keyed deduplication).

@Stebalien

This comment has been minimized.

Contributor

Stebalien commented Feb 22, 2017

The removal of caching seems fine on one of my computers. The caching really makes sense to me though, because I restart Emacs more often (every few days or so) than installing new apps (every few months or so). But if the overhead is so small, let's continue without caching for now.

It works fine with a hundred entries but slows down at 500-1000. I've implemented caching (with proper invalidation) but I'd like some feedback on it (there are many "correct" ways to do this).

counsel.el Outdated
(defvar counsel--linux-apps-cache-timestamp nil
"Time when we last updated the cached application list.")
(defun counsel-linux-apps-list-desktop-files ()

This comment has been minimized.

@Stebalien

Stebalien Feb 22, 2017

Contributor

I changed this to return a stable list of desktop files so we can detect changes with a simple (equals old-list new-list).

Stebalien added some commits Feb 20, 2017

Refactor counsel-linux-app.
* counsel.el (counsel-linux-apps-list): Remove the linux application
  cache (~counsel-linux-apps-alist~). Properly invalidating it (checking file
  modification times) would be almost as much work as recomputing it and
  computing it is instantaneous (on my system at least).

* counsel.el (counsel-linux-app-action-file): Don't try to put the application
  binary name in the prompt. Previously, this action would use the first
  space-delimited "word" of the =Exec= field. However, this isn't guaranteed to
  be meaningful. For example, WINE applications have =env= as the first word.

* counsel.el (counsel-linux-apps-list-desktop-files, counsel-linux-apps-list):
  Factor out the desktop file listing function into
  ~counsel-linux-apps-list-desktop-files~ and make it properly handle
  duplicate entries and subdirectories.
Don't accept empty fields in desktop files
* counsel.el (counsel-linux-apps-list): Use ~.+~ instead of ~.*~ to avoid
  capturing empty fields.
Use throw/catch instead of nested if statements.
* counsel.el (counsel-linux-apps-list): Use throw/catch to avoid nesting too
  deep.
Only parse "Desktop Entry" group
* counsel.el (counsel-linux-apps-list): Desktop files can contain multiple
  groups (one "Desktop Entry" and multiple "Desktop Actions"). Ensure we only
  look at the "Desktop Entry" group.
Hide linux applications when appropriate
* counsel.el (counsel-linux-apps-list): Hide applications when =Type= isn't
  =Application=, or =Hidden= or =NoDisplay= are =true= (or =1=).
Use call-process instead of call-process-shell-command.
* counsel.el (counsel-linux-app-action-default, counsel-linux-app-action-file):
  Use ~call-process~ instead of ~call-process-shell-command~ to avoid executing
  yet another process (=/bin/sh=) and to avoid potential code injection (when
  opening a file with a malicious name with ~counsel-linux-app-action-file~).
Support TryExec in desktop files
* counsel.el (counsel-linux-apps-list): Ignore desktop files where the program
  specified in the =TryExec= field either doesn't exist or isn't executable by
  the current user.
Make the linux application list generation O(N) instead of O(N*N)
* counsel.el (counsel-linux-apps-list-desktop-files): Use a hashmap to
  deduplicate desktop entries.
Add back counsel-linux-apps-faulty
* counsel.el (counsel-linux-apps-list): Use ~counsel-linux-apps-faulty~ to keep
  track of malformed desktop files. This way, we don't parse (and warn about
  them) multiple times. It may also make it easier to debug parser/desktop file
  bugs.
Actually search for desktop files recursively
* counsel.el (counsel-linux-apps-list-desktop-files): Use
  ~directory-files-recursivly~ instead of ~directory-files~ when searching for
  desktop files.
Add back the linux application cache.
* counsel.el (counsel-linux-apps-list, counsel-linux-apps-parse,
  counsel-linux-apps-list-desktop-files): Cache parsed desktop files and
  invalidate the entire cache whenever any desktop file changes. This becomes
  important on machines with many (~500) desktop entries.
@Stebalien

This comment has been minimized.

Contributor

Stebalien commented Apr 13, 2017

I believe this should be ready to merge when you are. FYI, in case you're waiting on it, I have signed all the copyright assignment paperwork (I can send you a copy if you want).

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Apr 14, 2017

Thanks. I see you in the copyright.list file already. It only remains to review the commits; there are a lot. One thing that I notice is that you use e.g. ~foo~ for quoting. In Elisp, quoting in docstrings is done:

`like-this'

And lists are not quoted at all in docstrings.

I'll fix the style myself in the merge, no need for further commits.

@Stebalien

This comment has been minimized.

Contributor

Stebalien commented Apr 14, 2017

@abo-abo abo-abo merged commit b941c5f into abo-abo:master Apr 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

abo-abo added a commit that referenced this pull request Apr 15, 2017

counsel.el (counsel-linux-app): Add "d" action to open desktop file
* counsel.el (counsel-linux-app-action-open-desktop): New defun.
(ivy-set-actions): Update.

Fixes #897
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Apr 15, 2017

Thanks for a great job!

But now that the applications list is in alphabetical order, I'll have to spend some time to get used to it.
Never noticed it was in reverse:)

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