Importer UI overhaul rebase/update

[davidswarbrick]

Description

Continuing work on PR #1685 implementing an improved UI, with more discussion/screenshots at #1593.
Previous work has been rebased on beetbox/beets/master. Currently I've fixed many of the tests which were failing due to the code targeting Python 2.7 (such as isinstance(basestring)) taking care to retain backwards compatibility so that we can get this into release 1.5.0. The current tests that are failing are asserting the output of the UI, I'm not sure if that's because they are yet to be updated for the new UI but that shouldn't be hard to address if so Update: The failing tests were due to a change in UI. I've adjusted the failing tests to match the new UI output, however there are extra tests that must be written to check, for example, Unicode output of the "not equals" sign if we wish to adopt the new UI.

Once all tests are passing a substantial reformat is needed to adhere to flake8 rules, I'd prefer to do this with black in one commit but that might be best to do after #3694 is merged.

To Do

• Rebased on current master branch
• Fix errors due to Python 2.7/3 discrepancies / internal logic
• Investigate whether failing tests need to be updated for new UI
• Review code for efficiency/legibility
• Add any extra tests for functionality not currently covered (e.g. construction of columns, Unicode output)
• Document functions & add whitespace etc. for compliance with flake8 (might be easiest to automate whitespace with black)
• Document differences to previous UI, perhaps with screenshots etc.
• Changelog - Add an entry to docs/changelog.rst near the top of the document

[jtpavlock]

Wow! This would be awesome to finally get finished! Thanks for taking this on.

Instead or targeting the ui branch, I think if you target master it will help you find actual conflicts and make the diff much more manageable

Edit: not sure if you can change the target branch on your end without opening a new PR or not. If you can't I can change it for you.

[RollingStar]

This is amazing.

[stale]

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.

[sampsyo]

I would love to keep this overhaul in play.

[stale]

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.

[sampsyo]

Yes, let's keep it open.

[davidswarbrick]

Tests passing include some minor changes to the old UI tests - there are some new bits of code around calculating columns which could do with testing.

Aside from the incomplete tests of the new functionality, the new ui is looking good! Example screenshot:

[davidswarbrick]

After some hefty re-working of the great work by @mxmerz (now over 5 years ago!) I'm hopeful that these changes are ready for a code review, and some documentation / publicity. If anyone would like to help add some screenshots from their setups it could be a good opportunity for non-code contributions to beets!

I've moved the vast majority of the new UI into the ChangeRepresentation class @mxmerz started, including making "newline" and "column" layouts into generic functions, which improves legibility and re-usability. This has allowed implementing column/newline layouts at the album level, not just for tracklists! We may wish to move those into ui/__init__.py in the future. I've added tests to check the column and newline wrapping works on each version of python, and documentation to the functions to explain their data structures and differences in output. As a consequence of "genericising" the layout printing functions, I've removed some of the custom data structures implemented originally which should help reviewers and future contributors alike understand what each function does.

The changes are best understood via show_change() and show_item_change() in beets/ui/commands.py which trigger the functions to print changes to the CLI. The docstrings for print_column_layout and print_newline_layout explain how the layouts are generated from the provided dictionaries. In the case of tracklists (which previously implemented a custom formatting structure) the function calls are now simpler: show_match_tracks -> make_line (for each track) -> print_tracklist (for all tracks) .
As always, happy for any comments or suggestions. Hope we can finally get this moving into main!

[sampsyo]

Absolutely incredible work! Thank you for reviving this effort. I would love to merge this in when we're confident in it!

Would you mind doing a little publicity to encourage some extra pairs of eyes on the new UI? As you mentioned, getting people to try it out and take screenshots would be an awesome non-code contribution to the project. Perhaps you could make a post or two in the GitHub Discussions area and/or on Discourse to help encourage people to take a look?

[seitzbg]

This is amazing, any ETA for a merge?

[stale]

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.

[davidswarbrick]

Hi, I'd hope this is still relevant, although this PR has indeed gone stale.
I've been using this on a personal branch for over six months now, but it would be good to get some other community involvement in the form of a review of the main structure.
Now that beets has moved on, the python 2-specific code can be removed, and the Black formatting should be rolled back as it unnecessarily increases the diff.

[davidswarbrick]

Rebased on latest master, and removed the unnecessary lines from the diff (blank lines or apostrophe substitutions). Happy for this to be reviewed and merged!

[wisp3rwind]

Rebased on latest master, and removed the unnecessary lines from the diff (blank lines or apostrophe substitutions). Happy for this to be reviewed and merged!

I have to confess that I still haven't tested this myself; but I'll try to gradually read the code, hopefully sooner than later.

One small thing upfront: I think we can drop all the explicit unicode prefixes u"..." for string literals, since Python 2 support was dropped in the meantime.

[wisp3rwind]

Added a first round of comments, see the inline notes.

[wisp3rwind]

... and a few more suggestions :)

[davidswarbrick]

Hi @wisp3rwind I've implemented the changes you suggested, apologies for the delay. This is ready for a re-review and (hopefully) merge.

[wisp3rwind]

Two small nitpicks on the new changes, otherwise they're looking great!

[davidswarbrick]

hey @wisp3rwind I have addressed the comments you made.
I see from #4488 and the discourse discussion of a roadmap that there are lots of great but longer-term ideas around UI/GUI/output rendering for beets, however I see this PR as a separate and completed improvement for short-term usability as we work towards new interface options, what do you think?

[Serene-Arc]

Wonderful! Congrats on really pushing to get this done, you got it to the finish line!

[Serene-Arc]

@JOJ0 The :: needs to be on a new paragraph in the section as I wrote, as it was. Otherwise it will add a colon to the end of the sentence, making it .:.

[JOJ0]

@Serene-Arc after resolving the conflict because of the just-merged #4547 I'm getting this error in the new UI. You know by heart if I failed with the conflict-resolving or if it really is an incompatibility with the new UI?

    Match (90.0%):
    Fat Freddy's Drop - LOCK-IN
    ≠ year, tracks
Disambiguation string key albumrelease does not exist.
    Spotify, None, 2020, None, THE DROP, None, None
    https://open.spotify.com/album/6D8fIhNTVPMe7DeOcJ4gfg
    * Artist: Fat Freddy's Drop
    * Album: LOCK-IN
       ≠ (#1) Special Edition (LOCK-IN) - F - 90 (7:31) -> (#1) Special Edition - LOCK-IN (7:31)

[Serene-Arc]

Almost certainly the latter. I wrote that PR so I'll take a look.

[Serene-Arc]

Might have to wait a few hours, Musicbrainz doesn't seem to be responding correctly. Either that or it's a bug with the new interface? I'm not sure.

[Serene-Arc]

Got it working, I can't for the life of me remember what that option was supposed to be but it doesn't exist on the album info object, so I removed it from the default configuration. In other words, all good now.

[JOJ0]

Got it working, I can't for the life of me remember what that option was supposed to be but it doesn't exist on the album info object, so I removed it from the default configuration. In other words, all good now.

Awesome, thanks for fixing so quickly! OMG, I'll hit that b....

[JOJ0]

OMG, 8 (in words: "Eight") years later....beets finally has an updated importer UI.

Thanks to @RollingStar, @Serene-Arc for the awesome teamwork! @sampsyo and @wisp3rwind, @arsaboo and @wazabees for testing, all the others I'm sure I forgot to mention that tested or sent feedback and everyone else I forgot!

But most of all, thanks to @mxmerz for the initial implementation, and @davidswarbrick for bringing it back to life and fixing some rather nasty, tricky bugs!!!

❤️ ❤️ ❤️

[arsaboo]

This is awesome guys....thanks to everyone who contributed to (and more importantly persisted with) this. Let us keep an eye out for bugs as more folks test the new UI.

[mxmerz]

Haha, so apparently it took eight (or more) people eight years to clean up my messy code 😉 But in all seriousness – thanks to you all! Truly incredible, I can't wait to try it out!

[davidswarbrick]

Thanks to all who contributed, reviewed, documented, reported bugs and fixed (& continues to fix) them! So excited to see these changes in the main branch and get them out into the wider beets community!!