Skip to content

fix(ux): Improve libzypp callbacks#1996

Merged
lslezak merged 12 commits intomasterfrom
improve_zypp_callbacks
Feb 19, 2025
Merged

fix(ux): Improve libzypp callbacks#1996
lslezak merged 12 commits intomasterfrom
improve_zypp_callbacks

Conversation

@lslezak
Copy link
Contributor

@lslezak lslezak commented Feb 14, 2025

Problem

Solution

  • Better button labels (now even translated!)
  • "Try again" is now the default
  • Display a title to have a context
  • Describe the consequences of skipping package installation
  • @dgdavid proposed even more enhancements (see his comment), but that would require non trivial enhancements

Notes

  • This is not a perfect solution, the current Query API is quite limited
  • I'll create a new card for enhancing the API to allow some more features (and remove the current workarounds)

Testing

  • Tested manually

Screenshots

agama-error-package-download
agama-error-package-integrity
agama-error-package-installation2

@lslezak lslezak added the ux UX related bugs/improvements label Feb 14, 2025
@coveralls
Copy link

coveralls commented Feb 14, 2025

Pull Request Test Coverage Report for Build 13386011811

Details

  • 31 of 33 (93.94%) changed or added relevant lines in 4 files are covered.
  • 43 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.06%) to 70.837%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/agama/software/callbacks/base.rb 13 14 92.86%
service/lib/agama/software/callbacks/provide.rb 8 9 88.89%
Files with Coverage Reduction New Missed Lines %
service/lib/agama/software/callbacks/progress.rb 1 68.89%
service/service/lib/agama/software/callbacks/provide.rb 1 94.44%
service/lib/agama/storage/finisher.rb 2 98.55%
service/service/lib/agama/storage/finisher.rb 2 98.55%
service/service/lib/agama/software/callbacks/progress.rb 7 68.89%
service/lib/agama/manager.rb 15 81.89%
service/service/lib/agama/manager.rb 15 81.89%
Totals Coverage Status
Change from base Build 13369642019: 0.06%
Covered Lines: 17771
Relevant Lines: 25087

💛 - Coveralls

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.

Taking a look to the screenshots in the description

  • I'd not use icon as part of the title But using it does not hurt and it is ok. Moreover, you have used the one used in the installation button when there are issues, which is great in terms of consistence.
  • I'd try to keep the buttons order. I.e, not adding the abort installation in the middle of the other two, which changes the order of buttons.
  • I'd use a "tertirary" or "plain" variant for the "Abort installation" action, if possible. Also, I'd add the "isDanger" prop.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Just some minor things. Otherwise, it looks much better now.

@lslezak
Copy link
Contributor Author

lslezak commented Feb 19, 2025

  • I'd use a "tertirary" or "plain" variant for the "Abort installation" action, if possible. Also, I'd add the "isDanger" prop.

This is a bit tricky. The problem is that the only safe option is "Try again".

Both "Continue anyway" and "Abort installation" can result in a broken or not bootable system. Although skipping a package feels a bit less dangerous in the end it does not matter if your system does not boot because of a single missing package or two dozens.

I do not want to create a false impression that skipping a package is a safe option. It's not. It's equally dangerous as aborting the installation.

Moreover this change would require non trivial enhancement of the QuestionActions component to allow specifying "danger" actions. And I'd postpone that for later, I think the current implementation is good enough.

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.

Let's merge it, thank you

@lslezak lslezak merged commit 03e4fdd into master Feb 19, 2025
13 checks passed
@lslezak lslezak deleted the improve_zypp_callbacks branch February 19, 2025 13:50
@imobachgs imobachgs mentioned this pull request Feb 26, 2025
imobachgs added a commit that referenced this pull request Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ux UX related bugs/improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants