Open Profpatsch opened 8 years ago
@mrmachine Oh yeah, (1) was an oversight – fixed it.
Regarding (2), I quickly threw together four options, see below.
+
for details, depending on the prompt d
might work better.Please discuss :wink:
@mxmerz Bear in mind that you only have 16 colors, we should really not go into “hard-code color values” territory.
Awesome! These are looking better and better.
I don't have a strong opinion between the mostly-blue or mostly-white styles. I but I do like the look of the new, verbose prompts—it's very valuable to make the interface legible for beginners and intermediates without an additional "help" option. The short prompt is quite aesthetically pleasing for an expert mode, so we should keep it in mind as an option.
Again, really love all the work you put into this. Just a few minor suggestions.
Although i like the idea of the
≠+-•
markers for changes, the way they are positioned now tends to make the visual overview of the section less clear. I wonder whether adding indentation for only the changes section by 3 to 4 characters would really hurt that much in exchange for a better overview.
Yes it's true that the release title with its 0 indent/reversed color scheme serve as a separator and all following release lines should be indented under it. I don't know which of minimum indent (2 chars) or more (4?) would give the best readability results.
Oh sorry, I already made mockups for that and forgot to upload them: 2 spaces, 3 spaces, 4 spaces, and an alternative with 3 spaces in front of and 1 space after the change marker.
I think I prefer three spaces.
Have no strong opinion on this, but prefer the 2 spaces for its simplicity (reduces number of different indent levels)
PS: out of curiosity, which tool you use to produce those mockups? coz u r pretty fast :dash: :rocket:
Instead of indenting the change section to match the title and supplementary information, I think I would prefer to reduce the indentation of the title and supplementary release information. If it must be indented, I agree with @Kraymer that 2 spaces is the best.
Any indentation at all (even 2 spaces) is going to make it more likely that track listings in 2 columns get wrapped more than they need to.
EDIT: Just to explain my reasoning, while I can see the utility in using indentation for blocks of related information within a match result (e.g. tracks), I don't see much point in indenting the entire match block. The entire block is already delineated from other match blocks by the match path which has reversed colours and blank lines around it.
Alright, I cloned beets yesterday and started on a ui
branch (see mxmerz/beets). It was a bit tricky, my Python skills are rusty at best, and the code quality is beyond hope – but:
The beets master branch for comparison: beets-ui-old1, beets-ui-old2, beets-ui-old3.
bold
, faint
, inverse
, background colors,…). To give you an idea, instead of yellow
, you would now write ['bold', 'yellow']
. I did not yet manage to make it backwards compatible, but with my naive worldview I think it should be possible support both string and array color definitions. This new approach makes it possible to define colors like ['inverse', 'blue']
(used for the path) or ['faint', 'bold', 'red']
or ['inverse', 'blink_slow', 'green']
(which I think would not be a good idea).
I would also be perfectly happy if we stay with the “old” approach to color definitions, it was just more easy to experiment with these.beets/config_default.yaml
.len(…)
, because it does not understand ANSI codes. The ugly hack I used to get around this was to drag along a colored and an uncolored version of every string through the whole process of aligning and layouting. Would have been a lot easier if there were a function that ignores ANSI codes when calculating string length. Is there?_colordiff
: Another ugly hack I had to do was to change the diff function to always end an ANSI color at a word border, jump over the whitespace and start coloring at the next word again. This was necessary because when I wrapped the titles on the left side, unclosed ANSI codes would color the rest of the line. The best option would probably be to take the whole word wrap code and make it understand ANSI codes (would also end the problems with len(…)
).Yeah. Again, feedback please! I’d probably go and spend time on improving code quality if you think it is worth continuing on this branch.
Awesome! Amazing work so far; this is not too far off from matching your hand-made mockups.
How do you feel about pulling this into a branch on the main repository? That way, we can collaborate in a central place—and even send pull requests to the development branch.
On word wrapping: Yes, this is a pain. In fact, you'll see we deal with exactly the same issue when wrapping difference displays by drawing along the length of the uncolored strings. As painful as it is, I actually think there's a chance it could get even more painful to use an ANSI-ignoring length calculation—adding in codes and then stripping them out again could become error-prone. Maybe we can find a better way to do this that computes the wrapping points in a first pass and then colorizes in a second pass.
A similar approach could help with multi-line colordiff
stuff. If we first find the wrap points, then the colorization positions, and then apply both to the string at once, we might be able to avoid most of the unpleasantness.
How do you feel about pulling this into a branch on the main repository? That way, we can collaborate in a central place—and even send pull requests to the development branch.
Yeah, sure. Just tell me what to do :)
And yes, I saw the wrapping code (I had to modify it to get the columns to align). If we are going to do more “advanced” layouts, measuring the length of strings will be probably something we have to do until right before we print them. Layouting first and coloring second would work in most cases (at least the ones I can think of) and I think that it would be an acceptable solution, but it would most probably require a more complicated colordiff
if we want to diff multiple lines as if they were one.
Do we want to ensure that whitespace gets diff’ed correctly? (By “correctly” I mean highlight the difference between tabs and spaces, or one space and two spaces, etc.)
@mxmerz: Tried your ui branch in Windows 10 X64 (yes, we still exist :-)), no errors, but also no ANSI goodness. Apparently, ANSI is by default not supported in the Windows cmd prompt. So I installed https://github.com/adoxa/ansicon, and tested it with http://adoxa.altervista.org/ansicon/ANSI%20Prompt%20Colours.txt: the ansi codes in the textfile get displayed nicely in my CMD-prompt, so I guess I got ANSI working in Windows now. Unfortunately, I still don't get them in beets. No idea what might be causing this. But maybe you want to take this into to consideration. It would be a shame if all your coding and designing efforts would be posix-only.
@ekjaker I don’t have a Windows machine to test this and I have no idea why this wouldn’t work. Does using the master
branch of beets give you any colors?
The only real difference between the ui
branch ANSI codes and the master
ones is that ui
uses more of them and that ui
produces, for example, \x1b[1m\x1b[32m
(bold
and yellow
) where master
might produce \x1b[32;01m
– I can change that in ui
to also use the shorter ones, but that shouldn’t make any difference in the output…
I actually do get colors, always have in the master
branch and still do in your ui
branch. Only stuff like backgrounds is not coming through, things I (possibly wrongfully) associate with ANSI.
Here's a screenshot of what it looks like:
But I'll do some more testing, try to narrow it down and get back to you hopefully with more information.
Seems like only the colors 31
-37
are recognized and not the other styles (bold, faint, inverse,…) – could you try this script (which tests more ANSI codes than the file you linked)? Should look something like this (the blink_slow
line should blink; my terminal doesn’t support italic
or blink_rapid
).
Currently the ui
branch uses styles bold
, faint
and inverse
. Background-colors and the other styles are implemented but not used with default preferences.
FWIW, on Windows, we use the Colorama library to translate ANSI color sequences into equivalent win32 calls.
OK, thanks, @mxmerz! I've pushed your branch to this repository and opened a pull request so we can track changes. If anyone wants to try out the work, you can just update your repository and git checkout ui
.
Do we want to ensure that whitespace gets diff’ed correctly?
Good question. We don't do anything about this currently, so for now at least we can ignore it. I'm more focused on making sure we keep the code maintainable.
It seems like colorama does not have support for styles other than colors (30
-39
), background colors (40
-49
), normal/reset (0
), bold (1
), and faint (2
) – especially not for inverse (7
), which I used to style the path.
They have an issue for this: tartley/colorama/issues/38
PS: For those interested, a list of ANSI codes can be found on Wikipedia (see Table “SGR (Select Graphic Rendition) parameters”).
@mxmerz
could you try this script (which tests more ANSI codes than the file you linked)? Should look something like this (the blink_slow line should blink; my terminal doesn’t support italic or blink_rapid).
The script you are referring to is a bash script, which unfortunately doesn't run in a windows command prompt.
It seems like colorama does not have support for styles other than colors (30-39), background colors (40-49), normal/reset (0), bold (1), and faint (2) – especially not for inverse (7), which I used to style the path.
On the same page, it is mentioned that another approach is to use ansi.sys:
An alternative approach is to install ansi.sys on Windows machines, which provides the same behaviour for all applications running in terminals. Colorama is intended for situations where that isn’t easy (e.g., maybe your app doesn’t have an installer.)
Ansi.sys doesn't work for windows 32 consoles, but the ansicon.exe I mentioned earlier does, and it also seems to support all ansi themes such as reverse. Maybe it is possible to use this instead of colorama, or at least give the option to Windows users to disable colorama and manually add ansicon.exe for better color support?
I just disabled the part in beets/ui/__init__.py
that imports colorama when windows is detected. As I have ansicon enabled, it now picks up on the ansi codes that colorama can not handle and normally silently removes, giving me what seems to be full ansi support:
Ansi goodness!! Look at all those pretty colors! :-)
PS. These screenshots are done in an 80 columns console.
I'm afraid I was a bit too optimistic: ansicon does support most but not all codes. Here's a list of what is supported. Unfortunately, 2 (faint) and 3 (italic) are missing. With colorama supporting 2 but not 4 (underline), 5 (blink), 7 (reverse) and 8 (concealed) this might become an annoying choice. But anyways, it is only a minor issue.
Interesting. It's too bad that installing ANSICON is so much less convenient than getting a shim from PyPI.
An alternative would be gracefully degrade and provide alternatives for inversion and such when they're not available.
Or we could keep it simple and just try to avoid anything colorama doesn't support. With the current design proposal, that means only the reverse
style, and that can easily be replaced by using background colors.
Only weird thing is that I can't seem to get black as text color working, it just reverts back to standard textcolor (as in the above screenshot), so a exact replacement it isn't yet. But other colors seem work just fine (screenshot below), So definitely no big deal, especially with all the customization options available.
are the colors colorblind friendly? I'm not colorblind, so I can't judge for myself
@jrobeson I not colorblind either, but isn’t the colorblind-compatibility an issue that has to be solved by the terminal emulator/theme?
Or as @Profpatsch was so kind to explain earlier in this discussion:
In modern terminals (read: 1970) there are 8 colors (white, cyan, magenta, blue, yellow, green, red, black) and each in two variations (dark and bright), leading to 16 colors in total. It’s essentially a color palette. So as a user you can still pick your favourite variants of colors (in your terminal config).
So as a programmer you basically only have to think about how humans perceive certain hues, e.g. red as a warning, yellow as attention seeking, green/cyan as accents. Or something along these lines.
Simply put: the colors are whatever you want them to be. Not only can you configure the basic color layout in the configfile due to mxmerz's awesome adjustments, you can change the interpretation of every color in your terminal.
BTW. I've been using the current proposal (the UI-thread) on a big import batch and the improvement is already HUGE!!! Really great design @mxmerz!
What is the status of this? Ignoring an obvious prerequisite to rebase and port over the changes, does something else need to be done? Is there anyone who doesn't like this new importer style?
That's a good question! I actually think the design is in great shape and don't have any specific suggestions for visual improvements. I'd love to see the patch updated so we can play around with it and make sure nothing's obviously wrong, then we can commit it!
If I remember correctly (that's a big if) my new code in #1685 worked, but was in dire need of a refactor and cleanup. And it failed a few tests. I don't think I'll have time/energy to finish it myself, but of course I'm happy to answer questions about it.
Potentially this work could be offloaded to an external library like rich
:
This would ultimately simplify the implementation a fair bit and allow to focus on the design and layout of the UI on our end. It would also remove the need to parse the colors on our end, since rich
has support for themes and flexible custom configuration.
To make the output to look like above, for example, I ultimately ended up reducing commands.py
by 50 lines.
Potentially this work could be offloaded to an external library like
rich
:
This would ultimately simplify the implementation a fair bit and allow to focus on the design and layout of the UI on our end. It would also remove the need to parse the colors on our end, since
rich
has support for themes and flexible custom configuration.To make the output to look like above, for example, I ultimately ended up reducing
commands.py
by 50 lines.
Is this work in a branch somewhere? would be nice to have an alternative UI overhaul approach going..
You can find this in the master
branch of my fork of beets.
There's a fair bit of other changes, but you may be able to isolate the changes in beets/ui/commands.py
and discard the rest. Though do use the poetry.lock
and pyproject.toml
to install the dependencies (the UI change depends on rich-tables repository which is where I keep some of the rich
utils I use).
Thanks @snejus, I will try rebase it on master in my fork and make a draft PR. Do you happen to have matrix? would be useful to ping you some questions.. You can reach me on tandy1000:matrix.org.
With the current formatting output, it’s extremely hard to find the crucial information. Current output might look like this (minus the colors):
I deliberately cut one output in half and used a terminal with half a screen’s width. The crucial information I need for a decision is:
Especially the first point is hard at the moment.
A few points to break it down
new entry
A newline would greatly increase readability.
paths
Since you invoke the import with
beets import <path>
, it would make sense to use relative paths to reduce clutter (and overlong lines).noise
Some chars are mostly noise after the first time:
Suggestion for reformatting
I’m not entirely happy with the last one, the list should be clearly visible and the prompt is two lines. Probably even the word
Similarity
should be taken out, the color-coded percentage is self-explaining.