| Summary: | JJ: Proper tooltips for Saved Playlists | ||
|---|---|---|---|
| Product: | [Applications] amarok | Reporter: | Alexander Potashev <aspotashev> |
| Component: | Playlists/Saved Playlists | Assignee: | Amarok Developers <amarok-bugs-dist> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | bart.cerneels, happy.snizzo, nhn, teo, vedant.kota |
| Priority: | NOR | ||
| Version: | 2.6-git | ||
| Target Milestone: | 2.8 | ||
| Platform: | Gentoo Packages | ||
| OS: | Linux | ||
| Latest Commit: | http://commits.kde.org/amarok/6ef8e597851e8e5638c7714c9b2649213db2732e | Version Fixed In: | 2.8 |
| Sentry Crash Report: | |||
| Attachments: |
screenshot
Change the playlist tooltip behaviour. Shows m_name instead of m_description. |
||
Setting the component correctly. 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. (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. (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? 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 :) Created attachment 69034 [details]
Change the playlist tooltip behaviour. Shows m_name instead of m_description.
Can you please submit the patch to https://git.reviewboard.kde.org ? Easier to review there. (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/ Is anyone working on this? If not I will pick it up (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:) (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 :) working patch uploaded. please review: https://git.reviewboard.kde.org/r/110082/ 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 |
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.