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

• [x] Rebased on current master branch
• [x] Fix errors due to Python 2.7/3 discrepancies / internal logic
• [x] Investigate whether failing tests need to be updated for new UI
• [x] Review code for efficiency/legibility
• [x] Add any extra tests for functionality not currently covered (e.g. construction of columns, Unicode output)
• [x] 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[bot]]

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[bot]]

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[bot]]

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.

[davidswarbrick]

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

[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?

[wisp3rwind]

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?

Hi, yeah, I didn't forget this PR; I'm just struggling to review it properly — in fact #4488 is not really a sign of big plans ahead, but merely an attempt to build a tool that help with the review of this PR.

I agree, this PR is more of an incremental change, and should be merged rather sooner than later. There seems to be another conflicting PR, #4473, which seems to be more disruptive (not sure; didn't read the code; also stalled for now) and might warrant more discussion on whether we want it.

[davidswarbrick]

Thanks @wisp3rwind. My understanding of #4473 is a helpful refactor of the UI library to make use of the extensive "rich" library, that seems to have stalled slightly also, much like this PR had for a time. It seems like an initially larger UI deviation but with benefits of reducing our internal complexity e.g. wrapping & colour handling.

In terms of user experience I hope this PR (#3721) provides a "ready" neater & not-too-dissimilar version of the original beets UI. The use of the "rich" library in #4473 does seem like a useful way to go as e.g. writing column wrapping code and tests for it does feel like outside the scope of the beets project, but as it isn't yet ready my proposal would be that we could bring in any community feedback on a new UI (good or bad) from the merge of #3721 into the work of the rich-restructure of #4473.

[JOJ0]

@davidswarbrick Hi! I changed this PR back to draft state even though it seems to be very close to be finished. Please set it back to "ready for review" if you find time to continue/finalize it. Hope that's ok for you and drop us a line any time! :-) Thanks for all your efforts!

[davidswarbrick]

Hi @JOJ0, it's quite frustrating to see this moved back to a draft PR. It is ready for review and has been for some time as the comments above indicate. My understanding is that the delay in merge is due to incomplete previous reviews. I've responded to the initial comments but have received no further review comments or points to address, so I don't understand what is required to get this merged, or whether there is any desire to do so. Please could you specify what you expect me to add?

[JOJ0]

Hi @davidswarbrick I'm very sorry, that was my bad! I didn't read thoroughly enough but saw unchecked boxes and currently am looking to tidy up the loads of PR's and try to find out what is stalling and why. If you think this PR is ready to review please set it back to that state. I really didn't want to add add to frustration your efforts are very much appreciated. The problem currently in the beets project is that most maintainers don't find time to review properly. I'm not sure how I can help at the moment but will think about it and bring up the topic again with maintainers!

[davidswarbrick]

I appreciate the explainer & apology, thanks @JOJ0, it's a huge project to work on and I'm grateful to all the maintainers who have given their time to look at this! I've just squashed and rebased on the latest master, please do let me know if there is anything I can do to help facilitate a code review - for example I'd be happy to get on a call with a maintainer if that would be helpful. The only outstanding checkbox is the changelog entry - I can draft something but I expect a UI change might come with a larger announcement. Since this PR has stalled for so long I haven't been sure if there's a reluctance to have such a large code change (as it clearly is difficult to review), which is why I haven't prepared that documentation, but if the maintainers would like to go ahead with this PR I'd be happy to flesh out the documentation and draft a release note.

[JOJ0]

Hi @davidswarbrick, I'm finally testing this UI in my daily workflows. I play with configurations right now. I don't get what the difference between column and newline layout is. Can you help me? What exactly would change if I do this for example:

ui:
    terminal_width: 40
    color: yes
    colors:
        action: ['bold', 'blue']
        action_description: ['normal']
        # import_path: ['bold', 'inverse', 'white']
        # import_path: ['normal']
    import:
        layout: newline

All the "action" and commented "import_*` things seem to work, I can see what's changing.

What should be different in newline layout? Or do I have a typo in my config maybe?

[JOJ0]

Ah, reading the docstrings of print_column_layout and print_newline_layout right now! The difference is documented here. Also I understand that terminal_width is a fallback only. To really see a wrapping, I need to adjust my actual terminal width. Ok ok, nevermind, thanks anyway!

[JOJ0]

I'm not sure, but I think I found an issue:

Seems like sometimes there are redundant whitespace characters thrown into the color-coded-text-diff (First and last of the 3 albums in the screenshot)

I'm working in column layout right now!

[davidswarbrick]

Thanks for the review! Yes I agree this looks like a bug, I think I noticed it too the other day but wasn’t sure if it was my terminal config. I’ll investigate further and update when I get the chance.

[davidswarbrick]

Hey @JOJ0, I've implemented some extra test suites which should catch the bug you found, they're passing now after some improvements, so if you get the chance it would be good to see if your testing works :smile:

[JOJ0]

Hi @davidswarbrick I have not forgotten about this PR. Thanks for the bugfix. I would like to finally integrate the current state into my personal main branch and try this new UI in my day to day use.

A couple of things that would help me move on:

• Could you rebase some of your latest commits into fewer commits so it's easier to cherry-pick. But please keep them separated if you think it makes sense and explains how and why you did things! Maybe just integrate the linting commits into the original "feature commits" or something like that. Tiny cleanups would help already!
• There is some UI code I was hacking on recently - it now conflicts with yours. It shouldn't be too much trouble to integrate it.
• If you don't find the time at all for this cleanup work - do you mind if I hijack your branch and do these things?

And then my further idea would be to get some more feedback:

• Create a beets discussion and call for beets users that want to test and give feedback on the new UI.
• I would provide a step by step guide on using your feature branch for testing, so the average beets user doesn't have to think too much about how to test and give feedback.
• I would just like to get a few more people to report that this is working fine for a few weeks (I actually did that already a couple of months ago and in my opinion this is working well, except that bug that you fixed recently)

In my personal opinion we can't test everything in the UI properly and sometimes have to rely on "real human testing" You did an awesome job on providing even more UI tests while fixing this issue!

[davidswarbrick]

Thanks @JOJ0 for the suggestions, I've squashed my recent commits into a single one representing the bugfix for the issue you found to help re-reviews, but feel free to hijack and squash further if we are to merge. I've rebased on the latest master, fixing the merge conflict you identified. I completely agree about "real human testing", but thanks for recognising the work finding edge-cases :smile: I'll draft a discussion post!

[davidswarbrick]

In response to user feedback from @arsaboo I've made a minor change to the default config which will improve readability in PuTTY Shells (as Windows users may use this or a derivative to access their beets instance). Those who wish to use the colour scheme from my previous screenshots can easily override the defaults in their personal config.yaml.