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

Order the actions listing by action/package #5045

Merged
merged 5 commits into from
Mar 7, 2022

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Feb 7, 2022

instead of by dependencies. A proposal related to #5041.

@AltGr
Copy link
Member Author

AltGr commented Feb 7, 2022

Here is what it looks like:
image

Repeating the logo at the beginning of each line seemed a little bit redundant, but if the header is out of the screen you won't see what action you are seing. Maybe a color bar on the left to delimit the block would help ?

Also it's slightly more verbose on some simple cases, but I don't think it's harmful, e.g. https://github.com/ocaml/opam/pull/5045/files#diff-b3f6abdb5ab3eb6c5612e441c0709542022442b9cc6f47b5eae75bc5aa85799cL82

@kit-ty-kate
Copy link
Member

mmh, I'm not sure about this form. It seems more confusing to me. e.g. if the solver decides to remove everything i won't be able see it's remove instead of recompile without scrolling up quite a long way.

I would much rather keep linear it as it is currently and simply reorder them

@kit-ty-kate
Copy link
Member

I had a shot at doing it based on what you’re doing in this PR (thanks!). In #5041 I was more looking at something like this:

Screenshot 2022-02-07 at 20 32 54

@dra27
Copy link
Member

dra27 commented Feb 7, 2022

I agree that repeating the logo is fine/good - it means you can "locally" (i.e. on the same line) see what's going on.

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 7, 2022

it means you can "locally" (i.e. on the same line) see what's going on.

Which further means it's grep and cut & paste friendly.

@rjbou rjbou added the AREA: UI label Feb 8, 2022
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 8, 2022
@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

New version, where I put back the "action symbols" at every line:
image

2 points I think are important:

  • changing the logics behind the message (i.e. the ordering) without making the output visibly different may be a bad idea, as it will defeat existing user's expectations without putting forward the parts that are more useful: hence the section headers to make the change clear
  • less line clutter means more easily parseable package names, so I prefer not to repeat the full action name on every line ? (except when utf-8 output is disabled at the moment)

Then again, it's just a personal feeling and UI is not my strong suit so please advise if you feel differently!

Ah also, I removed the "stats" at the end that showed the total for each action (in a somewhat cryptic form), thinking that was redundant. But maybe it remains useful if actions scroll out of screen ?

@kit-ty-kate
Copy link
Member

But maybe it remains useful if actions scroll out of screen ?

Yes exactly. I think this should be kept in at the very least.

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 8, 2022

But maybe it remains useful if actions scroll out of screen ?

Yes please !

(Also personally I don't need the == headers, I think @kit-ty-kate's version is fine, it also incurs less indentation which is good for your scarce horizontal space)

@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

(Also personally I don't need the == headers, I think @kit-ty-kate's version is fine, it also incurs less indentation which is good for your scarce horizontal space)

Hum, Kate's version as shown above uses more hspace since it repeats the action name on each line; also see my point 1. above, it's weird to change the way the message is built without any visual clue that it has changed.
Also, the indentation could be reduced one step... I don't know, screens are wide nowadays 🙃

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 8, 2022

Hum, Kate's version as shown above uses more hspace since it repeats the action name on each line; also see my point 1

You can omit that if you want. In general I find her design less agitated.

I don't understand what your point 1 refers to. If it refers to the fact that you are changing the design and you want existing users to notice, I'm not sure that's very useful. They will just read what is offered to them and this is what has to work.

I don't know, screens are wide nowadays 🙃

I wish people would stop making that assumption.

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 8, 2022

the action name on each line; also see my point 1

Personally I'm not disturbed by that but you could try to remove it from her design and have the action names next to the label in the summary, which would then double function as a legend for the diagram.

@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

I wish people would stop making that assumption.

Yeah that was a joke. My windows are set to stack horizontally by default…
I made an attempt to put the summary into the confirmation question, which I think goes better into the flow, what do you think ?

image

I don't understand what your point 1 refers to.

The fact that the actions were ordered topologically by dependency had some implications, which (I believe?) some may have gotten used to rely on. For example, in my screenshot above, bigarray-compat is the root cause of all the reinstallations, and that would have necessarily been above all of them, which is no longer the case. You can still look it up fairly quickly if you follow the uses xxx references… once you notice the list is now ordered alphabetically, which wasn't the case before and is not obvious without the separate headers.

@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

Another suggestion would be to delimit the actions by kind like what we already have here:
image

@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

(screenshot of the new version, with slight adjustments to make it feel less agitated)
image

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 8, 2022

think goes better into the flow, what do you think ?

I like that.

The fact that the actions were ordered topologically by dependency had some implications, which (I believe?) some may have gotten used to rely on.

Aha I never got that, or more likely I did rely on it without realizing.

It's true that with the new design you need to jump quite a bit around to find the root cause. Is it a good idea ?

@dbuenzli
Copy link
Contributor

dbuenzli commented Feb 8, 2022

Can we maybe highlight the root causes ? I.e. your example: color the [upstream or system changes] of bigarray.

@AltGr
Copy link
Member Author

AltGr commented Feb 8, 2022

Causes are

  | Use of 'a list
  | Required_by of 'a list
  | Conflicts_with of 'a list
  | Upstream_changes
  | Requested
  | Unavailable
  | Unknown

So the ones to highlight would be Upstream and Unavailable I guess ; given that Requested doesn't print anything ?

It's true that with the new design you need to jump quite a bit around to find the root cause. Is it a good idea ?

I just had someone suggest attempting to print a tree instead, e.g. something like

    /-∗ bheap                 2.0.0        [required by mirage-solo5]
    |-∗ metrics               0.3.0        [required by mirage-solo5]
    | /-∗ functoria-runtime   3.0.3        [required by mirage-runtime]
    |-∗ mirage-runtime        3.10.8       [required by mirage-solo5]
    | /-∗ ocaml-src           4.13.0       [required by ocaml-freestanding]
    |-∗ ocaml-freestanding    0.6.5        [required by mirage-solo5]
    | /-∗ conf-libseccomp     1            [required by solo5-bindings-spt]
    |-∗ solo5-bindings-spt    0.6.9        [required by mirage-solo5]
    ∗ mirage-solo5            0.6.5
      /-∗ conf-m4             1            [required by gmp-freestanding]
    /-∗ gmp-freestanding      6.2.1        [required by zarith-freestanding]
    ∗ zarith-freestanding     1.12         [required by mirage-crypto-pk]

… that could be helpful for understanding what's going on, but we probably won't want this by default (and that'll require someone interested in having fun coding it 😁 )

@rjbou
Copy link
Collaborator

rjbou commented Mar 2, 2022

Rebased

instead of by dependencies. A proposal related to ocaml#5041.

Actions output: more orderly and less indented
@kit-ty-kate kit-ty-kate merged commit d7a3863 into ocaml:master Mar 7, 2022
@kit-ty-kate
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants