Bug 308703 - Empty Folders in the Saved Playlists aren't preserved across Amarok restarts
Summary: Empty Folders in the Saved Playlists aren't preserved across Amarok restarts
Status: CONFIRMED
Alias: None
Product: amarok
Classification: Unclassified
Component: Playlists/Saved Playlists (show other bugs)
Version: 2.8-git
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: 2.9
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-20 12:06 UTC by Myriam Schweingruber
Modified: 2014-08-12 21:22 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Myriam Schweingruber 2012-10-20 12:06:19 UTC
When trying to create a folder in the saved playlist^merged view I see a "New Folder" appear on the screen, but the output only gives me

edit: editing failed

When I rename that folder, the result on the konsole out put is

X Error: BadWindow (invalid Window parameter) 3
  Major opcode: 20 (X_GetProperty)
  Resource id:  0x1800407

Once I quit Amarok and restart it the made folders are gone.
Comment 1 Matěj Laitl 2012-12-14 15:59:40 UTC
Bart, this goes to you much probably. I scent something horribly wrong in PlaylistsInFoldersProxy or directly in starting-to-smell-real-bad QtGroupingProxy. This is essentially the same bug as 310532 (also a release_blocker).

Because the "Playlists in folders feature" doesn't work for me at all (I haven't found a way to put a playlist into a folder) and the implementation is hacky, I'm thinking about removing it for 2.7, with a possibility for you to re-add it once fixed. Also, bug 301297 is closely related.
Comment 2 Matěj Laitl 2012-12-14 16:00:20 UTC
(In reply to comment #1)
> This is essentially the same bug as 310532 (also a release_blocker).

For the lazy: bug 310532.
Comment 3 Bart Cerneels 2012-12-17 09:57:43 UTC
(In reply to comment #1)
> Bart, this goes to you much probably. I scent something horribly wrong in
> PlaylistsInFoldersProxy or directly in starting-to-smell-real-bad
> QtGroupingProxy. This is essentially the same bug as 310532 (also a
> release_blocker).
> 
> Because the "Playlists in folders feature" doesn't work for me at all (I
> haven't found a way to put a playlist into a folder) and the implementation
> is hacky, I'm thinking about removing it for 2.7, with a possibility for you
> to re-add it once fixed. Also, bug 301297 is closely related.

Don't remove it. Would be a huge regression for users that organized their playlists this way. It has always worked for SqlPlaylists, but empty folders are not kept. This is because a folder in PlaylistBrowser is really a label on a playlist, i.e. a property of a playlist. If there is no playlist without that property there is no folder.
Comment 4 Matěj Laitl 2012-12-17 12:29:28 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Because the "Playlists in folders feature" doesn't work for me at all (I
> > haven't found a way to put a playlist into a folder) and the implementation
> > is hacky, I'm thinking about removing it for 2.7, with a possibility for you
> > to re-add it once fixed. Also, bug 301297 is closely related.
> 
> Don't remove it. Would be a huge regression for users that organized their
> playlists this way. It has always worked for SqlPlaylists, but empty folders
> are not kept.

Bart, are you actually able to put a Saved Playlist or a Podcast Channel into a newly created folder? With current git? I am not.

Bart, if you would like to keep this feature, please fix regression release_blocker bug 250062, which I am able to reproduce every time by creating 3 new empty folders, trying to drag some saved playlists to them (doesn't work, they apparently don't accept drops) and the quitting Amarok.

> This is because a folder in PlaylistBrowser is really a label
> on a playlist, i.e. a property of a playlist. If there is no playlist
> without that property there is no folder.

Yep, I understand this. After 2.7 is released, we should provavly change the UI to read "labels" or something similar. There seems to be some implementation of nested folders, which I cannot test, and which seems extremely sloppy to me - what is the satus of it?
Comment 5 Bart Cerneels 2012-12-17 20:20:45 UTC
I'll look into in during the next week while on the train. It usually is something stupid that I can fix easily once I understand the (miss)use of the proxy again.
Comment 6 Bart Cerneels 2012-12-24 11:36:26 UTC
Git commit cc88eb136229be323e4c6c76ecc13ac6cfbf3ceb by Bart Cerneels.
Committed on 24/12/2012 at 12:22.
Pushed by shanachie into branch 'master'.

Force editability of playlist folders.

M  +9    -0    src/browsers/playlistbrowser/PlaylistsInFoldersProxy.cpp
M  +2    -0    src/browsers/playlistbrowser/PlaylistsInFoldersProxy.h

http://commits.kde.org/amarok/cc88eb136229be323e4c6c76ecc13ac6cfbf3ceb
Comment 7 Bart Cerneels 2012-12-24 11:36:26 UTC
Git commit e1cad5e80f8f40b08c0c46d85afe0450db2c497c by Bart Cerneels.
Committed on 24/12/2012 at 12:33.
Pushed by shanachie into branch 'master'.

The correct edit() was not called, lacked polymorphism.

M  +1    -0    src/browsers/playlistbrowser/PlaylistBrowserCategory.cpp
M  +2    -1    src/browsers/playlistbrowser/PlaylistBrowserCategory.h

http://commits.kde.org/amarok/e1cad5e80f8f40b08c0c46d85afe0450db2c497c
Comment 8 Bart Cerneels 2012-12-24 11:40:51 UTC
Inline edits of folder names are still impossible.
This might be be a Qt bug:
https://bugreports.qt-project.org/browse/QTBUG-26838
I have not found a workaround yet.

I think it's not this bug though, inline rename is working for playlists.
Comment 9 Matěj Laitl 2012-12-31 16:17:14 UTC
Git commit 607ce0c3c3b89df3c16e19273e796092e26d156b by Matěj Laitl.
Committed on 31/12/2012 at 16:14.
Pushed by laitl into branch 'master'.

PrettyTreeView: proper polymorphism, revert e1cad5e80f8f40b0

Also revert "The correct edit() was not called, lacked polymorphism."

This reverts commit e1cad5e80f8f40b08c0c46d85afe0450db2c497c.

Bart, this is the right way to do it. Also, the polymorphism was not
the cause of failing edit, see the next commit.
CCMAIL: Bart Cerneels <bart.cerneels@kde.org>

M  +1    -2    src/browsers/playlistbrowser/PlaylistBrowserCategory.h
M  +6    -8    src/widgets/PrettyTreeView.cpp
M  +28   -24   src/widgets/PrettyTreeView.h

http://commits.kde.org/amarok/607ce0c3c3b89df3c16e19273e796092e26d156b
Comment 10 Matěj Laitl 2012-12-31 16:17:14 UTC
Git commit 369508c70b7d9ced37afc0ddfdb661d72d17c3ab by Matěj Laitl.
Committed on 31/12/2012 at 16:25.
Pushed by laitl into branch 'master'.

QtGroupingProxy: implement buddy() to work-around design issues

This is effectively a work-around for a big design flaw in
QtGroupingProxy (which is that it "invents" its own items not present
in the original model). Technical description in the code comments.

^^^^ Bart, because of the above reason I'd prefer not to use
QtGroupingProxy in Amarok at all in long-term. Instead, we should do it
the other way around: original model would be hierarchical and the
proxy would flatten it into a table.

This fixes some bugs in the Saved Playlists and perhaps more:

BUGFIXES:
 * Fix editability and drop-ability of playlist folders.
CCMAIL: Bart Cerneels <bart.cerneels@kde.org>

M  +1    -0    ChangeLog
M  +0    -1    src/browsers/playlistbrowser/PlaylistBrowserCategory.cpp
M  +23   -0    src/browsers/playlistbrowser/QtGroupingProxy.cpp
M  +1    -0    src/browsers/playlistbrowser/QtGroupingProxy.h

http://commits.kde.org/amarok/369508c70b7d9ced37afc0ddfdb661d72d17c3ab
Comment 11 Matěj Laitl 2013-01-02 16:33:02 UTC
Okay, this bug now boils down to preserving empty folders. This is a design issue of the Saved Playlists View: we internally store "labels" per each playlist, but represent them as folders in saved playlists. Because of that we have no way of storing a folder (label) with no associated playlist.

Because of the above, the bug cannot be solved for 2.7, isn't now a regression and doesn't block the release at all IMO.
Comment 12 Woodsman 2013-03-15 00:43:48 UTC
I would like to add that this bug affects populated folders too. I'm using 2.7.0 but noticed the problem in 2.6.0.

When displaying the list of saved playlists in merged view, I can add a folder, rename the folder, and move playlists into the folder. All looks well until I restart Amarok. The folders are gone and the saved playlists reverts to a flat list.

I miss this feature as this is the way I organized Amarok 1.4. I can "survive" with the flat list but having the folders is wonderfully convenient. :)

I don't have a local git repo but if patches are made available I can merge them into a fresh build and help test.
Comment 13 vedant agarwala 2013-05-30 20:23:11 UTC
A possible way to store folders without any playlists is to keep a "dummy playlist". This playlist will have the "label" as the name of the folder and a default name (or empty) and specific database ID common to all dummy playlists. The SqlPlaylistGroup will form the group as it does now. The PlaylistsInFoldersProxy will have to be changed to handle this accordingly and not display such playlists. It will only display the folder. Nested empty folders will be preserved in the same way.

If this seems like a good idea I will make a patch implementing it.
Comment 14 Mikhail Ivchenko 2013-12-03 22:03:49 UTC
Looks like it still not fixed in v2.8.0. Also when I am trying to delete 1 folder all folders are deleted.