Skip to content
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

Pattern selection #792

Merged
merged 27 commits into from
Oct 11, 2023
Merged

Pattern selection #792

merged 27 commits into from
Oct 11, 2023

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented Oct 4, 2023

Problem

  • User cannot change the default software selection in Agama, it always installs a minimal system
  • That is annoying for beginner users esp. in Leap or Tumbleweed installations where you want to have at least a minimal desktop system after installation

Solution

  • Allow selecting the available patterns, that's simpler and easier than a full package manager (for both users and developers)

Testing

  • Tested manually

TODO

  • Display solver errors if there are any
  • Unit tests
  • Comments in code
  • TRANSLATORS comments
  • Fix pattern group ordering to be the same as in YaST (needs discussion with @shundhammer, maybe later in a separate PR)

Screenshots

agama_patterns

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Coverage Status

coverage: 74.148% (-0.9%) from 75.084% when pulling 9b0e480 on pattern_selection into 898f681 on master.

@joseivanlopez
Copy link
Contributor

I think this is a movement in the good direction. Now it looks similar to the rest of pages. Personally, I would remove the first 4 lines, resulting something like:

[ filter ]

Graphical environments
[x] Gnome
[ ] KDE

Dev tools
[ ] Node JS

What do you think?

@shundhammer
Copy link

shundhammer commented Oct 4, 2023

@shundhammer
Copy link

Personally, I would remove the first 4 lines

I wouldn't remove the total size of the software to install. That affects remaining disk space for working with the system as well as download times.

As for the number of patterns to install, I don't know. It could be in one line to be a bit more concise:

Selected 4 patterns with 695.8 MiB total

Maybe the "Filter" heading could be a bit more subdued to avoid the impression that it's a category on the same level as the others; it's really just a tool to work on the others. Maybe a smaller font or a greyish text color would make that clearer (needs experimentation).

@lslezak
Copy link
Contributor Author

lslezak commented Oct 4, 2023

@lslezak: FYI: patterns have a (numeric IIRC) ZyppPattern::order() that is used for sorting them.

Yes, I wanted to implement the same also here. Sorting the patterns inside a group is trivial, I have problems with sorting the groups themselves.

In TW with Online repos I'm getting this order in YaST:

pattern_selector

While in Agama I'm getting:

Graphical Environments
Documentation
Base Technologies
Desktop Functions
Server Functions
Development
KDE Desktop
Budgie Desktop
Containers
Primary Functions
Proprietary Software
MicroOS
Kubernetes
Aeon

It seems that libyui should sort the groups according to the first pattern in the group (the pattern with the lowest order value). But for some reason the result is slightly different than expected and actually the Agama result matches that expected behavior.

In libyui for example the Documentation group (orders 1005 and 5200) is sorted between Base Technologies (orders 1030...5129) and Containers (orders 3000 and 9030) groups. Which looks strange, the final first pattern order is 1030, 1005, 3000.

I think it should be placed between the Graphical Environments (orders 1000...1802) and Base Technologies (orders 1030...5129) as in the Agama ordering.

@shundhammer any idea why the libyui sorting is like that?

(Note: the ZyppPattern::order() returns std::string so the values are compared lexicographically, not numerically. But that should not matter, all existing patterns use 4 digit values so that should work fine.)

@lslezak
Copy link
Contributor Author

lslezak commented Oct 4, 2023

I would remove the first 4 lines

The number of selected patterns is not that important, I'm fine with removing that. In the worst case you can still count them manually if you need that. But the installation size is important, it determines how much time the installation will take, how much data will be downloaded (if you have slow network or pay per megabyte) and the available free space after installation.

You can see that number in the overview dialog after going back, but I think it is better to see immediately how your each change influence the installation size.

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thanks a lot for taking care of this @lslezak!

I've done a review focusing mostly in the UI end result, omitting some parts I foresee will change in a near future like the styles for building the selector, the way we tell the user there is something auto-selected and few other minor things. As you told me, it's still not finished and, moreover, it's good enough for a first release that can be improved in next iterations.

Trying to keep it short, in no particular order

  • I wouldn't use title for the utility section. It is an introductory area of the Software page in which we can put useful information and some utils, like the search/filter box.
  • I'd avoid mixin the use of sections icons in a same Page. I.e., icons for all or for none. At least by now. See my comment in that regard.
  • Do not use a header for the filter. Just a plain label or we can even try to release it only with the placeholder.
  • Keep the Software overview section clickable always. The page should be aware if the client is busy and rendered itself in the loading mode. Users can land directly in https://agama/#/software.
  • Use the core/Section component as much as possible. Improve it if needed or create an issue if you believe these improvements do not necessary block this PR and are out of its scope.

I've deliberately skipped the logic part for building the whole page and making it work. No particular reason, I simply need more time for getting familiar with it and been able to come up with a good review/contribution. In any case, please add tests. They will help to prevent regression bugs and to have a big picture about pattern selection.

web/package.json Show resolved Hide resolved
web/src/assets/styles/app.scss Show resolved Hide resolved
web/src/components/overview/SoftwareSection.jsx Outdated Show resolved Hide resolved
web/src/components/software/PatternGroup.jsx Outdated Show resolved Hide resolved
web/src/components/software/PatternItem.jsx Outdated Show resolved Hide resolved
web/src/components/software/PatternSelector.jsx Outdated Show resolved Hide resolved
web/src/components/software/PatternSelector.jsx Outdated Show resolved Hide resolved
web/src/components/software/SoftwareSelectionPage.jsx Outdated Show resolved Hide resolved
@dgdavid
Copy link
Contributor

dgdavid commented Oct 4, 2023

Maybe the "Filter" heading could be a bit more subdued to avoid the impression that it's a category on the same level as the others; it's really just a tool to work on the others. Maybe a smaller font or a greyish text color would make that clearer (needs experimentation).

As commented, I think simply removing the header and using either, an HTML form label or only the text input placeholder, would be enough. Do not introduce more font sizes or color just for this.

@lslezak lslezak force-pushed the pattern_selection branch 10 times, most recently from e8ff8e9 to 687b0b5 Compare October 9, 2023 08:29
@shundhammer
Copy link

shundhammer commented Oct 9, 2023

JFTR: It turned out that libyui-qt-pkg had done this wrong; it always picked the last pattern for the order of its parent category, not the first one as it should.

The fix for that is on the way at libyui/libyui#107 (which also includes debugging aids such as making the order of each pattern and category visible in its own column when a environment variable is set).

@lslezak lslezak marked this pull request as ready for review October 10, 2023 08:53
service/package/rubygem-agama.changes Show resolved Hide resolved
web/package/cockpit-agama.changes Show resolved Hide resolved
* @param {JSX.Element} children the wrapped content with the patterns belonging to this group
* @returns {JSX.Element}
*/
export default function PatternGroup({ name, children }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: does it deserve its own component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally it was much more complex because it was a collapsible/expandable section. But I'd probably still keep it separate as it is a self contained logical part.

* @property {string} description long description of the pattern
* @property {string} order display order (string!)
* @property {string} icon icon name (not path or file name!)
* @property {number|undefined} selected who selected the pattern, undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: I usually indicate optional property instead of undefined, e.g. @property {number} [selected] - who selected .... We should agree a common format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied that from other code so it seems some unification is needed...

web/src/components/software/PatternSelector.jsx Outdated Show resolved Hide resolved
web/src/components/software/SoftwarePage.jsx Outdated Show resolved Hide resolved
@joseivanlopez
Copy link
Contributor

Just for the records: as discussed at some point, the selection icons should be revisited (something for a next iteration). Mainly for the auto-selected icon with subtle dots. Using labels was proposed as a possible solution.

Screenshot from 2023-10-11 09-58-14

@lslezak
Copy link
Contributor Author

lslezak commented Oct 11, 2023

Just for the records: as discussed at some point, the selection icons should be revisited (something for a next iteration). Mainly for the auto-selected icon with subtle dots. Using labels was proposed as a possible solution.

Yes, I just reused the same icons from YaST for the first iteration, I didn't want to reinvent the wheel. Later we can change that with something better.

@lslezak lslezak merged commit 99ab3e4 into master Oct 11, 2023
11 checks passed
@lslezak lslezak deleted the pattern_selection branch October 11, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants