-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add taskmanager config #103
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a configuration file for the Plasma Task Manager widget. The review identifies several areas for improvement to enhance usability and robustness. Specifically, some Enum type configurations should use named choices for clarity, another configuration entry lacks documentation for its possible values, and the default list of launchers contains hardcoded application-specific entries which may not be universally available, potentially leading to a poor default user experience.
| <entry name="groupingStrategy" type="Enum"> | ||
| <label>How tasks are grouped: 0 = Do Not Group, 1 = By Program Name</label> | ||
| <default>1</default> | ||
| </entry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The groupingStrategy entry is defined as an Enum, but it doesn't use <choices> to define the possible values. Instead, the values are documented as magic numbers in the label. This prevents configuration tools from creating a user-friendly selection list and forces users to remember the mapping between numbers and their meanings. To improve usability and maintainability, this should be defined with named choices.
| <entry name="groupingStrategy" type="Enum"> | |
| <label>How tasks are grouped: 0 = Do Not Group, 1 = By Program Name</label> | |
| <default>1</default> | |
| </entry> | |
| <entry name="groupingStrategy" type="Enum"> | |
| <label>How tasks are grouped</label> | |
| <choices> | |
| <choice name="DoNotGroup"/> | |
| <choice name="ByProgramName"/> | |
| </choices> | |
| <default>1</default> | |
| </entry> |
| <entry name="groupedTaskVisualization" type="Enum"> | ||
| <label>What happens when clicking on a grouped task: 0 = cycle through grouped tasks, 1 = try to show tooltips, 2 = try to show present Windows effect, 3 = show textual list (AKA group dialog)</label> | ||
| <default>0</default> | ||
| </entry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to groupingStrategy, the groupedTaskVisualization entry is an Enum that uses magic numbers documented in the label instead of named <choices>. This is not user-friendly and is error-prone. Using named choices would make the configuration clearer and more robust, especially for any UI generated from this file.
<entry name="groupedTaskVisualization" type="Enum">
<label>What happens when clicking on a grouped task</label>
<choices>
<choice name="CycleTasks"/>
<choice name="ShowTooltips"/>
<choice name="PresentWindows"/>
<choice name="ShowList"/>
</choices>
<default>0</default>
</entry>
| <entry name="sortingStrategy" type="Int"> | ||
| <label>Values from TaskManager::TasksModel::SortMode</label> | ||
| <default>1</default> | ||
| </entry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sortingStrategy is defined as an Int, and its label only refers to the C++ source code (TaskManager::TasksModel::SortMode) for possible values. This makes the configuration opaque and requires users to have access to and understand the source code to configure it. This is a significant usability and maintainability issue. This entry should ideally be an Enum with named choices, or at the very least, the label should document the meaning of the integer values.
| <entry name="launchers" type="StringList"> | ||
| <label>The list of launcher tasks on the widget. Usually .desktop file or executable URLs. Special URLs such as preferred://browser that expand to default applications are supported.</label> | ||
| <default>preferred://browser,applications:org.gnome.Ptyxis.desktop,applications:io.github.kolunmi.Bazaar.desktop,preferred://filemanager</default> | ||
| </entry> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default list of launchers includes hardcoded desktop file names for specific applications (org.gnome.Ptyxis.desktop, io.github.kolunmi.Bazaar.desktop). If these applications are not installed on a user's system, this will result in broken or missing icons in the task manager, leading to a poor default experience. The default configuration should be more generic and not rely on applications that may not be present.
<entry name="launchers" type="StringList">
<label>The list of launcher tasks on the widget. Usually .desktop file or executable URLs. Special URLs such as preferred://browser that expand to default applications are supported.</label>
<default>preferred://browser,preferred://filemanager</default>
</entry>
still need to test