feat: added launcher-cycle plugin to cycle launcher modes#329
feat: added launcher-cycle plugin to cycle launcher modes#329jakob1379 wants to merge 1 commit intonoctalia-dev:mainfrom
Conversation
|
first time making qml, so I am very eager to learn from feedback! |
spiros132
left a comment
spiros132
left a comment
There was a problem hiding this comment.
These are some questions / feedback I have about the PR :)
| id: root | ||
|
|
||
| property var pluginApi: null | ||
| property var cfg: pluginApi?.pluginSettings || ({}) |
There was a problem hiding this comment.
Since variables in qml are reactive (as far as I know), I would suggest you define all the variables that you're going to use up here, and then use them in the code. For example:
// Readonly so that the property changes depending on the values.
readonly property var additionalModes: pluginApi?.pluginSettings?.additionalModes ?? pluginApi?.manifest?.metadata?.defaultSettings?.additionalModes ?? ({})
// Or if you want to use the variables defined here
readonly property var cfg: pluginApi?.pluginSettings ?? ({})
readonly property var defaults: pluginApi?.manifest?.metadata?.defaultSettings ?? ({})
readonly property var additionalModes: cfg?.additionalModes ?? defaults?.additionalModes ?? ({})Note: Its better to use ?? instead of || since the latter can create false values if you have for example: || true, which will always be true, basically its better to get used to using ??.
| return result; | ||
| } | ||
|
|
||
| function getAdditionalModes() { |
There was a problem hiding this comment.
Basically as stated above, instead of having a getter function you can define all the variables up at the start to skip the extra code.
| return parseModeList(value); | ||
| } | ||
|
|
||
| function getAvailableModes() { |
There was a problem hiding this comment.
Btw another quick tip is that javascript code, can be used in the definition of a variable. For example:
readonly property var variable: {
if(...) {
return 1;
}
return 0;
}So the following function can be included in the variable definition.
There was a problem hiding this comment.
Is there a reason this recording also exists? You have the preview.gif
| property var cfg: pluginApi?.pluginSettings || ({}) | ||
| property var defaults: pluginApi?.manifest?.metadata?.defaultSettings || ({}) | ||
|
|
||
| property string editAdditionalModesText: modeListToText(cfg.additionalModes !== undefined ? cfg.additionalModes : defaults.additionalModes) |
There was a problem hiding this comment.
When choosing either the pluginSettings variable, or the defaultSettings variable it's always good to provide a fallback, for example ({}). If I'm not wrong it's because without a fallback there's a chance that a lot of warning errors can be printed, and that's not something we want in the logs, unnecessary warnings.
| if (isOpen) { | ||
| const currentIndex = modeIndex(currentMode, modeOrder); | ||
| if (currentIndex >= 0) | ||
| nextIndex = ((currentIndex + step) % count + count) % count; |
There was a problem hiding this comment.
I'm a bit confused what this calculation does, do you need to add count and then modulo with count two times?
| } | ||
|
|
||
| function parseModeList(value) { | ||
| var input = value; |
There was a problem hiding this comment.
Is there a reason why you create a variable called input here? Why not use the value variable from the parameters?
|
|
||
| var result = []; | ||
| var seen = {}; | ||
| for (var i = 0; i < items.length; i++) { |
There was a problem hiding this comment.
I'm guessing this part is for removing duplicates, there's an easier way to do it in Javascript and it's by utilizing a set, you can create a Set from the array and the convert the Set back to an array.
| 3. `>win` | ||
| 4. `>settings` | ||
| 5. `>emoji` | ||
| 6. `>clip` |
There was a problem hiding this comment.
Is there a command called clip in noctalia?
There was a problem hiding this comment.
yup
Works with multiple clipboard managers
|
Thank you for the review, really appreciate it. Will make sure to adjust soon. |
2 participants
Utility to cycle between launcher types with settings to blacklist and extend with custom modes.