Bug 275821 - JJ: Proper tooltips for Saved Playlists
Summary: JJ: Proper tooltips for Saved Playlists
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Playlists/Saved Playlists (show other bugs)
Version: 2.6-git
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: 2.8
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-16 15:56 UTC by Alexander Potashev
Modified: 2013-07-02 10:46 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.8
Sentry Crash Report:


Attachments
screenshot (97.12 KB, image/png)
2011-06-16 15:56 UTC, Alexander Potashev
Details
Change the playlist tooltip behaviour. Shows m_name instead of m_description. (3.16 KB, patch)
2012-02-23 13:40 UTC, Claudio Desideri
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Potashev 2011-06-16 15:56:08 UTC
Created attachment 61056 [details]
screenshot

Version:           2.4-GIT (using KDE 4.6.4) 
OS:                Linux



Reproducible: Always

Steps to Reproduce:
1. Run Amarok (I use Russian localization)
2. Create a playlist and save it in Amarok database
3. Switch Amarok to another localization ("Help" menu -> "Switch Application Language") and restart Amarok (I switched to US English localization)
4. Look at the tooltip of the previously created playlist, it's still in Russian (i.e. in the language you used when you created the playlist). Says "Список воспроизведения из базы данных", it's the Russian translation of "Playlist in database".


Expected Results:  
The tooltip should change according to the currently used localization.

The problem is that when you create a playlist, the _translated_ tooltip is stored in the database and does not change later. See src/playlistmanager/sql/SqlPlaylist.cpp:
1. at line 40, m_description is being set to the _translation_ of "Playlist in database" into the currently used language,
2. at lines 138 and 158 m_description get into the database via SQL queries.



The playlists I tested the bug with were created a long time ago, and now I'm using v2.4.1-264-gf0e247f. But I'm sure that the bug is still here, because I understand what the code from src/playlistmanager/sql/SqlPlaylist.cpp does.
Comment 1 Myriam Schweingruber 2011-06-16 16:52:22 UTC
Setting the component correctly.
Comment 2 Bart Cerneels 2011-06-16 20:47:04 UTC
This is normal. The names of the playlists are fully user determined. The pre-filled name is just a convenience and can't be made translatable.
Comment 3 Alexander Potashev 2011-06-16 22:40:28 UTC
(In reply to comment #2)
> The names of the playlists are fully user determined.

I'm talking about _descriptions_, not the names of playlists. AFAIK, the description can't be changed by user in the UI. There is even no such method in class SqlPlaylist that could set SqlPlaylist::m_description.
Comment 4 Bart Cerneels 2011-06-17 08:36:45 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > The names of the playlists are fully user determined.
> 
> I'm talking about _descriptions_, not the names of playlists. AFAIK, the
> description can't be changed by user in the UI. There is even no such method in
> class SqlPlaylist that could set SqlPlaylist::m_description.

Looks like m_description is legacy code that should be removed if we don't use it properly. It's only being set when creating a brand new playlist.

Here is your opportunity to determine some part of amarok: what do you think the tooltip should show?
Since you seem to know C++, perhaps you want to create a patch?
Comment 5 Claudio Desideri 2012-02-22 23:07:59 UTC
Here is my idea: we could just show the name of the playlist in the tooltip, as every other element do in other situations (e.g. in Local collection).

This is also not completely redundant, in fact if the user set a very long name which can't be fully displayed, the tooltip is useful.

I'm working on a patch for this. Tell me if something is wrong: first attempt to contribute :)
Comment 6 Claudio Desideri 2012-02-23 13:40:02 UTC
Created attachment 69034 [details]
Change the playlist tooltip behaviour. Shows m_name instead of m_description.
Comment 7 Teo Mrnjavac 2012-02-23 13:44:56 UTC
Can you please submit the patch to https://git.reviewboard.kde.org ? Easier to review there.
Comment 8 Claudio Desideri 2012-02-23 13:52:36 UTC
(In reply to comment #7)
> Can you please submit the patch to https://git.reviewboard.kde.org ? Easier to
> review there.

https://git.reviewboard.kde.org/r/104048/
Comment 9 vedant agarwala 2013-04-10 08:44:12 UTC
Is anyone working on this? If not I will pick it up
Comment 10 Myriam Schweingruber 2013-04-10 10:00:44 UTC
(In reply to comment #9)
> Is anyone working on this? If not I will pick it up

No, feel free to give it a try. I presume you have read all the documents linked from http://community.kde.org/Amarok/Development/Join:)
Comment 11 vedant agarwala 2013-04-11 07:59:02 UTC
(In reply to comment #10)
> No, feel free to give it a try. I presume you have read all the documents
> linked from http://community.kde.org/Amarok/Development/Join:)

Ok then I am picking it up. And Myriam I have read it thoroughly and already fixed a bug :)
Comment 12 vedant agarwala 2013-04-19 12:31:35 UTC
working patch uploaded. please review:
https://git.reviewboard.kde.org/r/110082/
Comment 13 Mark Kretschmann 2013-07-02 10:46:21 UTC
Git commit 6ef8e597851e8e5638c7714c9b2649213db2732e by Mark Kretschmann, on behalf of Vedant Agarwala.
Committed on 02/07/2013 at 10:36.
Pushed by markey into branch 'master'.

Proper tooltips for Saved Playlists; remove Playlist::description().

As agreed on the review for https://git.reviewboard.kde.org/r/104048/
Qt::TooltipRole has been updated so that now the tooltip displays full
name of the playlist. Occurrences of "description" have been removed
(from the Playlist base class as well as the subclasses).
FIXED-IN: 2.8
REVIEW: 110082

M  +12   -12   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp
M  +0    -1    src/core-impl/collections/mediadevicecollection/playlist/MediaDevicePlaylist.cpp
M  +0    -2    src/core-impl/collections/mediadevicecollection/playlist/MediaDevicePlaylist.h
M  +0    -15   src/core-impl/playlists/types/file/PlaylistFile.cpp
M  +0    -1    src/core-impl/playlists/types/file/PlaylistFile.h
M  +0    -1    src/core/playlists/Playlist.h
M  +0    -14   src/playlistmanager/SyncedPlaylist.cpp
M  +0    -1    src/playlistmanager/SyncedPlaylist.h
M  +4    -8    src/playlistmanager/sql/SqlPlaylist.cpp
M  +0    -2    src/playlistmanager/sql/SqlPlaylist.h
M  +3    -4    src/playlistmanager/sql/SqlPlaylistGroup.cpp
M  +30   -10   src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
M  +4    -0    src/playlistmanager/sql/SqlUserPlaylistProvider.h

http://commits.kde.org/amarok/6ef8e597851e8e5638c7714c9b2649213db2732e