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

single line transaction list #1191

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

long-walk
Copy link

The transaction list should easy to review.
Patch Benefits:

  • remove replacing line for its package (sub package still listed)
  • highlight arch, repo and size to see changes faster
    Screenshot_20240121_225751

@ppisar
Copy link
Contributor

ppisar commented Jan 23, 2024

I like your new format more than the current. But the old multi-line format was there for some reason. I think @j-mracek should be able to explain the reason.

@long-walk
Copy link
Author

long-walk commented Jan 23, 2024

I know there are lines like this brown and this are still present.
image
This time with Standard colors title and major changes are normally white, Sorry I had a modified color theme in my terminal. Maybe we should change this in dnf5 to blue?

@j-mracek
Copy link
Contributor

The transaction table is a tricky thing. It contains operation install, remove with only one version and upgrade that contains in general two versions. But there are other combinations. like upgrading package might replace multiple packages and installing package might also replace multiple packages. Then a combination of the one version showed in version column and others in new lines might be confusing.

There is additional requirements

  • the format must look OK with 80 character terminal. This is a use case for container images (build), because 80 is still a good default (this might be problematic with this PR)
  • Number of columns must be stable (the PR fulfill the requirement)
  • Table must contain all in an out packages (the PR fulfill the requirement)

PS: This PR is the best approach how to reduce lines in output, but still I am not sure whether it will be OK for 80 characters terminal.

@long-walk
Copy link
Author

long-walk commented Feb 3, 2024

It is not possible to put all this output to one line in this small terminals so dual line still need to be support. The actual dnf5 seems also to struggle with the output on small terminals. I did some testing and this looks mostly good expect some edge cases if the package name or version are to long then they could be truncated at the end.

  • Adjust output to fit 80 character terminal. I add a limit of 130 char to change between dual line and single line
  • Version highlighting use lightblue for epoch major/minor version changes (commit below)
  • The size highlighting is adjusted, smaller package need factor 10 change bigger package need 10% to be highlighted
  • comparison previous dnf5 (fc40) with PR
    Screenshot_dnf5-80-compare2
  • 129 character
    Screenshot_dnf5-129_v2
  • 130 character
    Screenshot_dnf5-130_v2

@long-walk
Copy link
Author

Hello @j-mracek
Thanks for your replay, abovee is my proposal can you give me a feedback for this.

@Conan-Kudo
Copy link
Member

Where is <unknown> coming from in the repository column?

@long-walk
Copy link
Author

is based from my testing in a virtual system normally you will see the new source.

@j-mracek
Copy link
Contributor

Where is <unknown> coming from in the repository column?

There has been a problem with transfer information from DNF4 but is is fixed now - #1403

@m-blaha
Copy link
Member

m-blaha commented Apr 23, 2024

@j-mracek we should decide on this. Recently I've been working on adding skipped packages into transaction table and I'm afraid that PR I plan to submit (currently draft, it needs some testing #1440) will cause bigger conflicts with this one.

@jan-kolarik jan-kolarik self-requested a review April 25, 2024 12:38
@Conan-Kudo
Copy link
Member

The main thing I think it missing is highlighting vendor changes, which is the main detail missing for #750.

@jan-kolarik
Copy link
Member

Related issue: #326.

@long-walk
Copy link
Author

long-walk commented May 15, 2024

@m-blaha this is the merged version with the #1440 is this the output you would expect?

fedora40# build/dnf5/dnf5 upgrade --releasever=38
dnf5_1191

@carlwgeorge
Copy link

The right-to-left format of the version column is odd to me. It's rather confusing for the version change to be formatted in that direction. Left-to-right would make much more sense and match the rest of the output.

I'm not sure there is much value in having old and new values in the repository column. I think that one should just show the repository that the update is coming from. In most cases, that is going to be the same name written twice, i.e. updates -> updates or appstream -> appstream. Similarly, I don't think it's worth the space taken to show old and new values for the size column.

@m-blaha
Copy link
Member

m-blaha commented May 16, 2024

@m-blaha this is the merged version with the #1440 is this the output you would expect?

The first column looks too wide for me - like it contained the section titles. How does it look with 80 cols terminal?
This is a transaction with broken dependency package on narrow terminal using the latest upstream dnf5:

Screenshot_20240516_073556

@jan-kolarik jan-kolarik linked an issue May 16, 2024 that may be closed by this pull request
@jan-kolarik
Copy link
Member

jan-kolarik commented May 16, 2024

I tested this a bit, and here are my thoughts:

  • The 80-column output is better than the current state in the main branch.
  • If there's a hard-coded limit for the version column, it might look odd in some cases. We should check if both versions fit, and if not, place them on separate lines (see image below).
  • When a package replaces something with a different name, the "replacing" line should be indented (see also image below).
  • I am also in favor of changing to a left-to-right format.

In general, we should decide if there will also be a verbose version of the transaction table (e.g., when -v is passed). In that case, we can include all the info there. Otherwise, if there will only be one version of the output, I'd omit the repo and size changes.

Overflowed versions with COPR packages
image

Non-indented replacing lines
Screenshot from 2024-05-16 13-01-42

@long-walk
Copy link
Author

  • The intention to do the right to left was that the new Version that will installed is the first and line up with the other versions from skipped packages, dependencies packages and Replacing packages. Maybe it could also be the arrow that you have a problem with, would bracket change your mind?
    image
    With Breakages

  • The first column has a dynamic size if possible with a bit of priority to have a small gap after it.

  • The size of the column are variable in length and this is done by libsmartcols library. I warp it manually but their is also the option to do it automatic but that can result in multiple lines (Screenshot above) in small windows you have all information but you lose clarity.
    With 80 cols it looks not good for me if I warp the text automatic. If I truncate the Version it looks better but you lose some information. The Package name will never warped or truncated because that locks strange. Example:
    image
    image
    Maybe I truncate the data below 130 cols and warp it above.

  • The non-intended rplacing line must be may fault during the merging and is fixed.

  • The dnf5 Release version shows all the sizes and repository so I tried only to remove the duplicate information for me the -v verbost options sounds good but at the moment I couldn’t figure out how this works I need some more code reading.

Test commands I use:
dnf5 upgrade --releasever=38
dnf5 upgrade --releasever=41

@carlwgeorge
Copy link

carlwgeorge commented May 18, 2024

Maybe it could also be the arrow that you have a problem with, would bracket change your mind?

No, brackets don't change my mind on this. I think most people will view it in a linear way, left to right, old to new. Showing the new version first is going to lead to confusion. If the goal is to vertically align new versions, then I think the best approach is to have an old and new version column, using n/a as needed. This is what I originally proposed in #326.

When we are restricted to an 80 col terminal, I think the easiest fix is to just suppress the architecture and size fields. Those are much less important than everything else. Here is an example of what that could look like for various types of transactions.

Package                        Old Version         New Version        Repository
Upgrading:
 foo                           1.0-1.fc39          1.0-2.fc39            rawhide
Downgrading:
 foo                           1.0-2.fc39          1.0-1.fc39            rawhide
Installing:                                                                     
 foo                           n/a                 1.0-2.fc39            rawhide
Removing:                                                                       
 foo                           1.0-2.fc39          n/a                   rawhide

It even works with a multi-replace transaction.

Package                        Old Version         New Version        Repository
Upgrading:                                                                      
 foo                           n/a                 1.0-1.fc39            rawhide
   replacing bar               1.0-1.fc39          n/a                   rawhide
   replacing baz               1.0-1.fc39          n/a                   rawhide

@Conan-Kudo
Copy link
Member

I think in constrained rendering, we should just drop the old version info or maybe move it to a new line saying it replaces the old version. We also still need some indicator of vendor changes somehow, but that could probably be plugged into the same rendering logic for obsoletes.

@jan-kolarik
Copy link
Member

jan-kolarik commented May 29, 2024

This is a complex topic, and not everyone will be satisfied. Our team discussed this and agreed to make minimal changes to the current default transaction table layout. For now, the primary concern mentioned repeatedly is fixing the "replacing" lines for identical package names. Additionally, we need to highlight vendor changes, as Neal mentioned.

For a more verbose version, it would be better to enable it explicitly via a CLI or configuration option, rather than automatically based on window width. This approach will also simplify the fixes in our CI tests stack, where we parse the transaction table output. I've created a separate issue to follow up on the ideas presented here and continue the discussion.

Thanks to everyone, especially @long-walk, for initiating and working on this sensitive topic. 🙂

@long-walk
Copy link
Author

Hello @jan-kolarik
I still worked a bit on this PR but as more as I change as more problems it has in edge cases. The last state looked bad with 80 cols because it uses to much space with two Version columns.

Only for reference the last state (code is not in state to push).
not verbose and Truncate data if to long:
image

verbose Warp data:
image

So then I think we can close this PR and I will look in the progress of #1518

Thanks for your help.

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.

7 participants