Add user-configurable distribution path prefixes (implements #822)#1961
Add user-configurable distribution path prefixes (implements #822)#1961JonasPfi wants to merge 2 commits into
Conversation
|
Undeclared use of AI, please read and understand the AI policy. Review your code and only then submit PRs. |
|
I added what I used to the commit. Since its my first contribution I used Claude mostly to get used to the repo and also for some logic. |
|
Did you see the "review your code" part? |
|
I've reviewed all changes. I removed a comment I missed earlier but did not find anything else. |
|
My question is: Why do we need this? Even though I believe there are demands for a custom list of path prefixes, I don't think the need for a UI for it is guaranteed. |
| typedef struct StringItem_ { | ||
| OptionItem super; | ||
| char** ref; | ||
| bool editing; | ||
| bool valid; | ||
| LineEditor editor; | ||
| bool (*validate)(const char* text); | ||
| } StringItem; |
There was a problem hiding this comment.
I'm skeptical of this item. It looks like a duplicate functionality when we have "Screens" whose names are editable.
There was a problem hiding this comment.
Could you clarify. Do you mean that StringItem is unnecessary as a new abstraction, and we should instead embed LineEditor directly in DisplayOptionsPanel like ScreensPanel does?
There was a problem hiding this comment.
No. I mean there's a significant overlap between the two string editing functionality. It's better to consolidate the two functionality into one code. How to do that is left for you to figure out.
There was a problem hiding this comment.
Uhh. Forgot to mention this: LINEEDITOR_MAX is currently 128 (bytes). This is like not enough for editing a list of strings.
There was a problem hiding this comment.
Agreed. I'll hold off on refactoring until there's consensus on whether the feature is wanted.
| bool NumberItem_addChar(NumberItem* this, char c); | ||
| void NumberItem_deleteChar(NumberItem* this); | ||
|
|
||
| StringItem* StringItem_newByRef(const char* text, char** ref, bool (*validate)(const char* text)); |
There was a problem hiding this comment.
Split the bool (*validate)(const char* text) function definition to its own typedef, please?
| bool editing; | ||
| bool valid; | ||
| LineEditor editor; | ||
| bool (*validate)(const char* text); |
There was a problem hiding this comment.
Ditto. (Split the bool (*validate)(const char* text) function definition to its own typedef.)
We do not encourage editing
Inside |
|
Thank you for your review @Explorer09, @JonasPfi you see you have lots to do. |
|
@BenBE, @natoscott, @cgzones: |
|
Yes, this might need more than the line editor and a full editor panel would probably not be worth it. |
|
Another advantage for using an external list file is that distributions can ship with a preconfigured list of paths, freeing the user for needing to configure the list themselves. If distributions want to ship with a preconfigured list, this list will likely reside in |
|
That would still be in a distro-default |
Um... Putting a settings file inside (If we ever need a system-wide htoprc, it would be |
|
Use the source, Luke. It is what we do. And have been doing for decades. |
| } | ||
|
|
||
| NumberItem* numItem = (OptionItem_kind(selected) == OPTION_ITEM_NUMBER) ? (NumberItem*)selected : NULL; | ||
| StringItem* strItem = (OptionItem_kind(selected) == OPTION_ITEM_STRING) ? (StringItem*)selected : NULL; |
There was a problem hiding this comment.
RTTI (run-time type information). This is a smell of bad object/class modeling. Unfortunately I don't have any idea to fix it for now.
| return len; | ||
| token = *end ? end + 1 : end; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
There's still a performance concern with this new function. The old code had a switch-case that could make the lookup slightly faster, but now we're forced to perform a linear search...
I wish a slightly faster data structure such as a trie.
There was a problem hiding this comment.
In this specific case implementing a trie might be overkill given the small number of prefixes. A full implementation would add significant complexity for what I'd expect to be a minimal performance gain.
There was a problem hiding this comment.
I dont know. If it's me I'd implement it. Otherwise I don't like to trade a performance loss for a feature that few people use.
Assisted-by: Claude (Anthropic)
| } else { | ||
| const char* val = (this->ref && *this->ref) ? *this->ref : NULL; | ||
| if (val) { | ||
| RichString_appendAscii(out, valAttr, val); |
There was a problem hiding this comment.
Don't use RichString_appendAscii for strings read from external sources. Use RichString_appendWide instead.
Assisted-by: Claude (Anthropic)
| const StringItem* this = (const StringItem*)cast; | ||
| int labelAttr = CRT_colors[CHECK_TEXT]; | ||
| int boxAttr = CRT_colors[CHECK_BOX]; | ||
| int valAttr = this->valid ? CRT_colors[CHECK_MARK] : CRT_colors[FAILED_READ]; |
There was a problem hiding this comment.
What does the validate function inside a StringItem do? And what would happen (or should happen) if the validation fails?
There was a problem hiding this comment.
Currently its a placeholder. There was the idea of validating if the input path is a valid path in #822 but I did not implement any validation.
There was a problem hiding this comment.
And what should be done if the "validation" fails?
There was a problem hiding this comment.
The input should be displayed in red as visual feedback.
There was a problem hiding this comment.
The "red" visual feedback would be not good enough if you have a list of path prefixes to edit. Also, I saw no code the "discard" directories that don't exist or are exact duplicates of previously specified paths.
| StringItem* StringItem_newByRef(const char* text, char** ref, bool (*validate)(const char* text)) { | ||
| StringItem* this = AllocThis(StringItem); | ||
| this->super.text = xStrdup(text); | ||
| this->ref = ref; |
There was a problem hiding this comment.
What does ref do here and how is this argument different from text?
There was a problem hiding this comment.
text is the label displayed next to the input field ( "- Custom path prefixes"). ref is a pointer to the settings field being edited (&settings->distPathPrefixes), so changes are written directly to the settings struct.
| this->editBuffer[this->editLen] = '\0'; | ||
| } | ||
| } | ||
| StringItem* StringItem_newByRef(const char* text, char** ref, bool (*validate)(const char* text)) { |
There was a problem hiding this comment.
typedef bool (*ValidateStringFn)(const char* str, size_t len, void* context);There was a problem hiding this comment.
Will consolidate into a typedef. Will use your suggested signature with len and context if the feature moves forward.


Fixes #822, depends on #637
Problem
The 'Shadow distribution path prefixes' feature (#637) uses a hardcoded list of paths (
/usr/bin/,/bin/,/nix/store/,/run/current-system/, etc.) that cannot be changed by the user. This makes the feature unusable for non-standard setups such as Flatpak (/var/lib/flatpak/), custom install prefixes, or other distributions with different filesystem layouts.Solution
This PR implements two things as requested in #822:
1. A new
StringItemsettings controlExtends the existing settings UI infrastructure (
OptionItem) with a new text input type backed by the existingLineEditor. This gives users a readline-like editing experience with support for:←/→,Ctrl+A,Ctrl+E)The
StringItemfollows the same patterns as the existingCheckItemandNumberItemcontrols.2. User-configurable path prefix list
Replaces the hardcoded
CHECK_AND_MARK_DIST_PATH_PREFIXESswitch macro inProcess.cwith amatchesDistPrefix()function that checks against a user-supplied colon-separated list of paths. When the list is empty, the original built-in defaults are used as fallback.Changes
OptionItem.h/.c— newStringItemtype withLineEditorintegrationSettings.h— newchar* distPathPrefixesfieldSettings.c— read/writedist_path_prefixesfrom/to htoprcDisplayOptionsPanel.c— new text input field below the existing checkboxProcess.c— replace hardcoded switch macro withmatchesDistPrefix()Testing
valgrind --leak-check=fullreports 0 definitely lost bytes