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

Importer UI overhaul #1685

Closed
wants to merge 45 commits into from
Closed

Importer UI overhaul #1685

wants to merge 45 commits into from

Conversation

sampsyo
Copy link
Member

@sampsyo sampsyo commented Nov 2, 2015

This is mxmerz/ui, pulled into the main tree. See screenshots and such at #1593.

- Add ANSI styles, colors and background-colors
- Modify colorize functions to adopt new colors
- Modify default configuration to adopt new color scheme:
   - Define colors by arrays with their ANSI codes as strings
- Deprecated DARK_COLORS and LIGHT_COLORS
for easy indentation and alignment
- when linebreaking, ANSI codes should end at word borders
- this removes change markers for whitespace!
- colors arbitrary strings according to a distance
- refactor dest_string
- reintroduce support for legacy colors
- replace DARK_COLORS and LIGHT_COLORS with LEGACY_COLORS and map color names onto their counterpart in the new “advanced” color definition system
- also continue to support new “advanced” colors
- cleaner code (at least I think so)
@sampsyo
Copy link
Member Author

sampsyo commented Jan 9, 2016

Hi! This was looking pretty close to ready for beta-level publicity. Shall we regroup and finish it off? Maybe we should start by making a to-do list of outstanding changes.

@mxmerz
Copy link

mxmerz commented Jan 9, 2016

I’m very sorry for the long silence from my side – I had a busy November and December and am currently in my exam period… I hope to be able to help with this again in 1-2 weeks. From what I immediately remember, the dirty code I hastily hacked together worked okayish, but was severely lacking in quality, simplicity, documentation, and tests. And I am sure that the new UI is not showing changes for some tags, eg. track length, or when the chroma id didn’t match.
Sorry again for this half-assed amateur work, if no-one else dares to touch it I promise to get to it as soon as my exams are over and I have a bit more free time.

@sampsyo
Copy link
Member Author

sampsyo commented Jan 9, 2016

Fantastic! And no problem whatsoever—open-source hacking necessarily has to take second place behind exams. Thanks for keeping it in mind, and no rush at all.

I'll let you take the lead, but I'm happy to pitch in with code quality and such.

@ekjaker
Copy link

ekjaker commented Jan 13, 2016

And maybe I can help out with some testing. I have been using these modifications for a large tagging job, and it worked nicely. As it is now, it's already a huge improvement.

@sampsyo
Copy link
Member Author

sampsyo commented Jan 13, 2016

Awesome! Yes, testing is great. Please do post if you run into any problems.

Adjusting the unit tests for the new UI would be helpful too.

@mxmerz
Copy link

mxmerz commented Jan 27, 2016

I have a few new commits over at mxmerz/ui.

The “old” color definitions with a single string should work again.

I tried to refactor the show_change function in ui/commands.py – I split the function into show_match_header, show_match_details, and show_match_tracks and tried to split these again into smaller functions. My python skills are limited; I am not sure whether this is the best way to approach this and would appreciate feedback before I continue.

In ui/__init__.py, I added a function uncolorize which strips ANSI codes from a string and color_len, which measures the length of a colored string (len(uncolorize(string))). In uncolorize, I used a StackOverflow-inspired regex, do you know whether that is compatible with beet’s license and correctly attributed? I actually hope to get rid of both functions again, it does not seem to be a very elegant way…

@sampsyo
Copy link
Member Author

sampsyo commented Jan 27, 2016

Great! This looks like solid progress.

I sent you an invitation to join the @beetbox organization, which should let you merge your changes into the ui branch on this repository. Would you mind doing that?

And while uncolorize looks like a useful stopgap, I agree with you about eventually finding a solution that doesn't require it. Ideally, we'd be able to do things like getting the length of the string once, before adding colors, and avoid the cost of colorizing and then re-de-colorizing to recover the length.

- move construction of lhs and rhs dicts to make_line
- move make_track_titles, make_track_numbers, and make_track_lengths to make_line
 - add 'changed' key to info dict to indicate whether rhs should be displayed at all
@GuilhermeHideki
Copy link
Member

For the colorize problem, you could subclass or composite str or unicode and create a field with the length, and in a colorize method, you would "overwrite" the uncolorized string with the colorized.

Create color mapper functions outside of the forloop.
Instead of sometimes moving the last word in the last line, just always add an empty line as the new last line.
The class should make it easier to handle state when showing a change.

- Move show_match_header and show_match_details to the new class.
@jtpavlock
Copy link
Contributor

Anything else that needs to be done here?

@sampsyo
Copy link
Member Author

sampsyo commented Jul 11, 2020

I think what this needs is some good old fashioned TLC—taking some time to review everything in detail and think carefully about the changes. I think the outcome (i.e., the resulting UI) already looks awesome.

@stale
Copy link

stale bot commented Nov 18, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 18, 2020
@stale stale bot closed this Nov 25, 2020
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