xdg-terminal-exec: Misc changes #46
Conversation
|
I like everything except removing config validation errors. |
|
After sleeping on it, I think I agree with you, let me fix that real quick |
|
I think that rebase worked, can't see any obvious issues with it |
| debug "$entry_id : $action_id" | ||
| ;; | ||
| # Catch and complain on invalid lines | ||
| *) |
There was a problem hiding this comment.
this is too aggressive, we need to silently drop unrecognized stuff, only react on invalid entries (things containing .desktop, but otherwise invalid).
There was a problem hiding this comment.
Would you rather it be a debug message, or nothing at all?
There was a problem hiding this comment.
I liked validate_entry_id(), validate_action_id(), we could use them if line contains .desktop and produce error on negative result. And just do not bother with anything not explicitly recognized and not containing .desktop.
There was a problem hiding this comment.
...not an error, a warning.
There was a problem hiding this comment.
I like this more because all the logic is in one place
Simpler to write tests for too
I changed the catch all to a debug message, is this good?
There was a problem hiding this comment.
On the topic, as the file format is part of the proposed spec, I think the reference implementation should be responsible for validating the files. At least to some degree
This at least catches all the cases I could think of A fun bonus feature, there now can be whitespace between the IDs, allowing for more readable list files
Less code While here, improve comments and order of checks Makes it look prettier
Uses the given action ID as a flag to signify that the Exec value should be read, by checking if it's set or not One less variable to carry around
Makes writing tests more straight forward and allows hiding checks conditionally This makes carrying `$de_checks` unnecessary
The spec refers to them as identifiers, makes it clearer what the variable holds
Now that they're separated, these describe them slightly better Also touch all relevant comments / debug messages
Various changes in an effort to make `DEBUG=1` output more readable
| 'TryExec') check_entry_tryexec "$value" ;; | ||
| 'Hidden') check_entry_hidden "$value" ;; | ||
| # Only read key from requested action group, marked by unset ID | ||
| 'Exec') [ -z "$action_id" ] && check_entry_exec "$value" ;; |
There was a problem hiding this comment.
This fails on any explicitly specified action, needs to be wrapped in full if then fi
| debug "found '$line' directive${XTE_CACHE_ENABLED+ (ignored)}" | ||
| debug '' " $config_path" | ||
| # Let `read` trim leading/trailing whitespace from the IDs | ||
| while IFS=":$OIFS" read -r entry_id action_id; do |
There was a problem hiding this comment.
this splits action not only on :, but on whole original IFS, falsely consuming invalid lines
See commit messages for details
These should again be the last changes I had queued up
Last two commits, b7f7682 and dfc44b1, are more opinionatedI'd like to see them merged, but feel free to cherry-pick them out if you so choose 👍