Implement new context view with metadata
ClosedPublic

Authored by mgallien on Feb 2 2019, 4:51 PM.

Details

Reviewers
januz
ngraham
Group Reviewers
VDG
Maniphest Tasks
T10330: Implement new context view from mockup
Commits
R255:3239643239b5: change the way lyrics and other metadata are shown and improve lyrics
R255:7457ddfe956d: fix ContextView to no longer generate errors when shown
R255:951771dbc620: ensure metadata is refreshed in context view when track changes
R255:83a262ab9825: do not always put a value for disc number when the metadata is not here
R255:4c74ea0bc24c: Implement new context view with metadata
R255:8ff4b7195ad8: when showing ContextView, fix width to not overflow
R255:c263529ed32e: allow to inherit from TrackMetadataModel to filter properties
R255:ffe312f1bfd5: remove useless debug prints
R255:d5bbc17e7f30: remove obsolete methods from TrackMetadataModel
R255:c3cb65b73996: Implement new context view with metadata
R255:0e9abefea16a: add possibility to handle Lyrics KFileMetaData property
R255:90f77ec8da3f: fix TrackMetadataModel to not create multiple connections
R255:b466fbab044d: in the detailed metadata view compose the property name with i18nc
R255:8687684e2ab3: enlarge the context view to fit new content
R255:1322313da888: fix size of the cover in context view
R255:2bf9cc7c808f: adapt context view to tracks missing album name or artist name or both
R255:0193e73cd742: introduce TrackContextMetadataModel to display metadata in context view
R255:3645b1f3a244: provide new context view with metadata from currently playing track
R255:8c75d1209692: fix update of metadata model when a track change
R255:a79247b6100c: add the filename at bottom of context view
R255:b92110827ee6: do not display disc number when only one disc
R255:5d1b080c3727: DatabaseInterface::trackHasStartedPlaying notify modified tracks
R255:67042c8fcd8e: add support for fetching lyrics in TrackMetaDataModel and display it
R255:bddd9d74d640: allow ManageHeaderBar to export a databaseId property for current track
Summary

enlarge the context view to fit new content

allow ManageHeaderBar to export a databaseId property for current track

in the detailed metadata view compose the property name with i18nc

partial upgrade of the database to better handle some missing metadata

do not always put a value for disc number when the metadata is not here

add back indexes when recreating Tracks table

allow comment of tracks to be null

store some metadata only if they are really defined

only really return data on tracks if they are not null in database

allow to inherit from TrackMetadataModel to filter properties

remove obsolete methods from TrackMetadataModel

introduce TrackContextMetadataModel to display metadata in context view

fix TrackMetadataModel to not create multiple connections

provide new context view with metadata from currently playing track

remove useless debug prints

fix size of the cover in context view

when showing ContextView, fix width to not overflow

add the filename at bottom of context view

fine tune the display of context view to be with regular spacing

Test Plan

context view mostly look like the design done by @januz

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mgallien requested review of this revision.Feb 2 2019, 4:51 PM
mgallien created this revision.

Cool stuff! I gave it s shot but was unable to play any of my music. All the artists show up in the Artist view, but if I go into any of those artists, they have no albums or songs, and Album view and Tracks are likewise empty.

If I go to the Files category and navigate to a song in my music folder and try to play it, I get the following console spew:

qrc:/qml/ViewSelector.qml:53:9: QML ScrollView: Binding loop detected for property "contentHeight"
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/unsynch.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/zero-length-mdat.m4a"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/zero-size-chunk.wav"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/005411.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/empty-seektable.flac"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/excessive_alloc.aif"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/broken-tenc.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/segfault.aif"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/segfault.oga"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/infloop.wav"
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
qrc:/qml/PlayListBasicView.qml:125:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:124:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:121:17: Unable to assign [undefined] to QUrl
qrc:/qml/PlayListBasicView.qml:120:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:119:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:118:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:117:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:115:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:125:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:124:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:121:17: Unable to assign [undefined] to QUrl
qrc:/qml/PlayListBasicView.qml:120:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:119:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:118:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:117:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:115:17: Unable to assign [undefined] to int

Also there's an i18n() error on the Now Playing panel when nothing is playing:

Cool stuff! I gave it s shot but was unable to play any of my music. All the artists show up in the Artist view, but if I go into any of those artists, they have no albums or songs, and Album view and Tracks are likewise empty.

If I go to the Files category and navigate to a song in my music folder and try to play it, I get the following console spew:

qrc:/qml/ViewSelector.qml:53:9: QML ScrollView: Binding loop detected for property "contentHeight"
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/unsynch.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/zero-length-mdat.m4a"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/zero-size-chunk.wav"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/005411.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/empty-seektable.flac"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/excessive_alloc.aif"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/broken-tenc.id3"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/segfault.aif"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/segfault.oga"
Unable to open file:  "/home/nate/kde/src/taglib/tests/data/infloop.wav"
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/TextField.qml:57:5: Unable to assign [undefined] to QQmlComponent*
qrc:/qml/PlayListBasicView.qml:125:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:124:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:121:17: Unable to assign [undefined] to QUrl
qrc:/qml/PlayListBasicView.qml:120:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:119:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:118:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:117:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:115:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:125:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:124:17: Unable to assign [undefined] to int
qrc:/qml/PlayListBasicView.qml:121:17: Unable to assign [undefined] to QUrl
qrc:/qml/PlayListBasicView.qml:120:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:119:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:118:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:117:17: Unable to assign [undefined] to QString
qrc:/qml/PlayListBasicView.qml:115:17: Unable to assign [undefined] to int

Also there's an i18n() error on the Now Playing panel when nothing is playing:

Thanks for your report.

I will see if I can open a new review to really improve the database situation with regard to the kind of problem you are having.

In the meantime, I will fix the problems with tracks coming from the file explorer and possibly having few metadata.

mgallien updated this revision to Diff 50905.Feb 5 2019, 5:59 AM
  • adapt context view to tracks missing album name or artist name or both

partially fix the problems reported by @ngraham

Thanks, I can now browse songs in the file browser. However Elisa still isn't finding any songs in the primary (only) configured location (~/music). I see this forever:

Thanks, I can now browse songs in the file browser. However Elisa still isn't finding any songs in the primary (only) configured location (~/music). I see this forever:

I am working on a solution to ensure that the user has a solution when music scanning is not working (i.e. the Elisa music cache/database is corrupted).

mgallien updated this revision to Diff 51809.Feb 15 2019, 10:35 PM
  • adapt context view to tracks missing album name or artist name or both

rebase on top of master that now included a feature to re index music

Your latest commits to master cause Elisa to at least try to rebuild the library, but it gets stuck after a while. Ideally it would re-use the existing library, and if it absolutely needs to perform a re-index, it shouldn't get stuck. I've filed https://bugs.kde.org/show_bug.cgi?id=404458 to track this so we can stop cluttering up the review comments for this patch.

mgallien updated this revision to Diff 53961.Mar 15 2019, 3:16 PM
  • adapt context view to tracks missing album name or artist name or both

rebase on top of master

mgallien updated this revision to Diff 54816.Mon, Mar 25, 9:10 PM
  • adapt context view to tracks missing album name or artist name or both

rebase on top of master

mgallien updated this revision to Diff 55116.Sun, Mar 31, 9:16 AM
  • adapt context view to tracks missing album name or artist name or both

rebase on master

seems to crash when switching to context view

mgallien updated this revision to Diff 55235.Mon, Apr 1, 9:14 PM
  • do not display disc number when only one disc
  • DatabaseInterface::trackHasStartedPlaying notify modified tracks
  • fix update of metadata model when a track change
  • fix ContextView to no longer generate errors when shown

Fix most issues I could have seen during testing.
@ngraham could you have a look when you have time to do it ?

Pretty great. Could the context view also show the lyrics from the currently playing song's lyrics tag (if present?). That would make this a CCBUG: 401969 :)

Pretty great. Could the context view also show the lyrics from the currently playing song's lyrics tag (if present?). That would make this a CCBUG: 401969 :)

Thanks for the suggestion I will have a look at it for the next days.

mgallien updated this revision to Diff 55864.Tue, Apr 9, 10:02 PM
  • adapt context view to tracks missing album name or artist name or both
  • do not display disc number when only one disc
  • DatabaseInterface::trackHasStartedPlaying notify modified tracks
  • fix update of metadata model when a track change
  • fix ContextView to no longer generate errors when shown
  • add possibility to handle Lyrics KFileMetaData property
  • add support for fetching lyrics in TrackMetaDataModel

Add the possibility to display Lyrics from a track.

I need feedback on real lyrics because my music does not have them as existing metadata.

Awesome! Here's how the new lyrics feature looks with some random songs of mine:


Some feedback:

  • The "Lyrics" label should be top-aligned, not vertically center-aligned
  • The view containing lyrics needs to be vertically scrollable
  • In case the individual lines of the lyrics are very long, the font should automatically reduce in size so nothing is ever cut off and no horizontal scrolling is ever needed or wanted
  • When the Now Playing view is open and a new song is played, the title updates, but the metadata does not
mgallien updated this revision to Diff 56095.Fri, Apr 12, 7:46 PM
  • adapt context view to tracks missing album name or artist name or both
  • do not display disc number when only one disc
  • DatabaseInterface::trackHasStartedPlaying notify modified tracks
  • fix update of metadata model when a track change
  • fix ContextView to no longer generate errors when shown
  • add possibility to handle Lyrics KFileMetaData property
  • add support for fetching lyrics in TrackMetaDataModel
  • improve display of long lyrics by adding a vertical scroll bar
  • ensure metadata is refreshed in context view when track changes

improve management of lyrics

Thanks, this is much better. I feel like the lyrics view is almost there.

This version scrolls vertically, but still lets the lyrics get cut off horizontally for very long lines:

I wonder if it would make sense to remove the lyrics section from the metadata formlayout, and display it below the metadata. That would get rid of the large left margin and make it less likely that long lines will get cut off. Sort of like this:

If you go this route, then center-justifying the text would probably make sense.

Also, there's a funny issue with the new vertical scrollbar:

(Ignore the screen tearing; that's not Elisa's fault!)

mgallien updated this revision to Diff 56099.Fri, Apr 12, 8:55 PM
  • add support for fetching lyrics in TrackMetaDataModel and display it
  • ensure metadata is refreshed in context view when track changes

Use word wrap to allow long lines to be fully shown

I will now work on your feedback. By the way thanks a lot for it.

You're very welcome. Would you like me to start submitting some patches for the bug reports I filed today, or is the UI considered to be in a state of flux such that I should hold off on it?

mgallien updated this revision to Diff 56248.Sun, Apr 14, 9:30 PM
  • adapt context view to tracks missing album name or artist name or both
  • do not display disc number when only one disc
  • DatabaseInterface::trackHasStartedPlaying notify modified tracks
  • fix update of metadata model when a track change
  • fix ContextView to no longer generate errors when shown
  • add possibility to handle Lyrics KFileMetaData property
  • add support for fetching lyrics in TrackMetaDataModel and display it
  • ensure metadata is refreshed in context view when track changes
  • change the way lyrics and other metadata are shown and improve lyrics

Improve display of lyrics (and fix funny scrollbar by the way)

ngraham added a reviewer: VDG.Mon, Apr 15, 2:45 AM
ngraham accepted this revision.Mon, Apr 15, 2:47 AM

UI looks fantastic to me now. You fixed all the bugs I found, and I couldn't find any new ones. :) Might be better to get this into master and iterate on it there rather than let it languish in review for longer.

This revision is now accepted and ready to land.Mon, Apr 15, 2:47 AM
This revision was automatically updated to reflect the committed changes.