-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WFindOnWebMenu: Menu for to find track properties in online music databases #4772
Conversation
src/widget/wtrackmenu.h
Outdated
@@ -10,6 +10,7 @@ | |||
#include "library/trackprocessing.h" | |||
#include "preferences/usersettings.h" | |||
#include "track/beats.h" | |||
#include "track/track.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of including the entire header here, can Track be forward-declared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I have deleted this line.
Did you read https://doc.qt.io/qt-5/signalsandslots.html or similiar ressources on signals/slots? Also, please rebase this onto main to resolve the merge conflicts. |
src/widget/wtrackmenu.cpp
Outdated
@@ -949,6 +963,12 @@ void WTrackMenu::slotOpenInFileBrowser() { | |||
mixxx::DesktopHelper::openInFileBrowser(locations); | |||
} | |||
|
|||
void WTrackMenu::slotFindOnSoundcloud(const Track& track) { | |||
const auto artistName = track.getArtist(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use
TrackPointer pTrack = getFirstTrackPointer();
to obtain the track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That worked.
Thank you :)
src/widget/wtrackmenu.cpp
Outdated
QDesktopServices::openUrl(QUrl("https://soundcloud.com/search/people?q=")); | ||
void WTrackMenu::slotFindOnSoundcloud() { | ||
const TrackPointer pTrack = getFirstTrackPointer(); | ||
QString artistName = pTrack->getArtist(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be empty. Use album artist as fallback?
If that is also empty, ask the user? Or just open the soundcloud search and let the user type there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, album artist and asking the user might be good option. I can implement that, If it's okay.
Right now it opens SoundCloud with a blank page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to % encode the artist name.
Opening SoundCloud with a blank page sounds good enough for me, to realize that the artist is empty.
What exactly do we want to ask the user? What will be the alternative? We may gray out the menu entry or open the track property dialog to add an artist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to % encode the artist name.
Oh I just realized that QUrl is smart enough to do it automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do we want to ask the user? What will be the alternative? We may gray out the menu entry or open the track property dialog to add an artist.
Graying out the menu action is not helpful IMO. Initially I thought of a simple textbox dialog asking for the track artist, apply that to the track, then attempting to open soundcloud again. But that's too cumbersome UX-wise and to program for this first step.
Since an empty soundcloud query would not solve the missing tag issue we could show a dialog
This track has no artist or album artist tag.
Try again after filling the tag(s) in the track properties dialog or inline in the tracks table.
[ Open track properties] [ Okay ]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it simple without requiring any additional user input, see the "Search related Tracks" feature. If I need to type I could open a browser myself.
Mixxx should not become a launcher for external applications. Opening the file explorer is a notable but useful exception. If a feature can't be integrated properly into Mixxx it should not become part of Mixxx. The feature as currently planned is already an edge case IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked at "Search Related Tracks" feature it is so nice. When there is no artist info. It doesn't show the find artist row. This feature can be like this too, so no user input needed and also can be extended, such as find the artist, albumartist, track on lastfm discogs or musicbrainz in a single menu such as "Search on web".
Opening the file browser is a must and it is very useful. Launching the web browser can be useful too. I think for some users It can be helpful to find the artists on different services. If they are looking for information on the web a lot about the artist, such as latest releases, albums etc. It can save their time.
...and make CI work. |
Thanks for the feedback. I have read the exact page. I couldn't resolve the problem. I was facing with this error when I tried to get Track info by reference. Otherwise, there was not any error. But this time I couldn't get the trackinfo. I tried to collect the track by looking to other slots but couldn't collect it in that way too. Now I got the artist by pointer. As you mentioned, I have disabled the multiple tracks and rebase the branch onto main. |
Signal and slot need matching arguments (or you use a slot that doesn't expect arguments at all, like your current slot). |
Ah, That's right. Now I got it why I was having this error and couldn't resolve it. Thank you :) |
src/widget/wtrackmenu.cpp
Outdated
void WTrackMenu::slotFindOnSoundcloud() { | ||
const TrackPointer pTrack = getFirstTrackPointer(); | ||
QString artistName = pTrack->getArtist(); | ||
QDesktopServices::openUrl(QUrl("https://soundcloud.com/search/people?q=" + artistName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend not to use naive string composition for creating URLs, see QUrl::setQuery()
.
Please also externalize all the business logic into separate classes/files, don't bother WTrackMenu
with this extra stuff. It is already too big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, I will take a look at QUrl::setQuery() and work on business logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey again,
I was trying to move all the logic into other classes and I was working on QUrl::setQuery()
. I just have a question,
I have a function, which takes 2 arguments one is the URL itself and the second one is query (in this case, Artist, Album artist, Album, Track title). My Url is https://www.last.fm/search/artists?q=
and the artist is Eminem
.
When I call the method QUrl::setQuery()
, it works just fine, the Url is created. When the Url is opened I realized the opened Url is https://www.last.fm/search/artists?Eminem
which opens a page without any results. 'q=' part is gone.
Should I pass all the queries with the prefix "=q" into my function or should I use another method which fits into key value pattern?
Because when I looked into documentation it says about 'setQuery'.
This function is useful if you need to pass a query string that does not fit into the key-value pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the Qt doc, For me it seems like you should use QUrlQuery and then use the QUrl::setQuery(QUrlQuery)
overload. QUrlQuery:
The QUrlQuery class provides a way to manipulate a key-value pairs in a URL's query. More...
The QUrl class does much more than just concatenate strings. When you pass it an a URL with a query and then call setQuery
, it will overwrite the previous query, which is why setQuery("Eminem")
results in the previous query (q=
being overwritten). Since you are dealing with key-value pairs here, use QUrlQuery
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback.
Now I realized that I should use both the QUrlQuery and setQuery. For two different things.
If pagination is simple and I can directly open the artist-track-album page. I can simply use setQuery. Because there is no key and value pairs. There will be no queries.
If we can't open artist-track-album page directly and if we need to do search on the service, I should use QUrlQuery, to have key q= and the value artist-track-album name.
0ea8a6a
to
c9d4f08
Compare
You branch looks like that in gitk. Luckily git has a solution:
I use for conflicts SmartGit which has a nice three files diff view for that. |
Ah you have solved the problem and squashed everything to a single commit. Great. |
Works good! |
Hey everyone! We have discussed with my mentor about this PR and feature. Either it is merged or not it can be a playground to learn the Mixxx code base for me, so I would like to work on it a bit more. One of the most important thing is WTrackMenu class is too big and we don't want to bother it a lot. To do that we want to use factory design pattern for FindOn feature. So with that, we can have separate files. Expand this feature with different websites. It can be turned on or off on the preferences menu. But I am wondering how users can use this feature, open the default browser? Shouldn't they right click on the track, on the TrackMenu and then choose either one of the websites? I should still use the WTrackMenu class to call FindOn right? I don't want to bother WTrackMenu class a lot. I wonder how will it look like on the user perspective. Instead of QUrl I will try to use QUrl::setQuery(). Should I also keep going on this PR, or on a brand new branch and PR? |
IMHO As long as this state does not break something in main we could merge in steps of less perfect solutions. But wat means good enough here? Ideas? |
I am unsure where the FindOn feature should be hooked. My original request, a Discogs lookup for possible genres and the first time release date would be also a good fit in the track property dialog. The same is also true for cover art look up ... Both locations will finally work. Ideas? |
Finding the artist/track site on audio platforms (Soundcloud, Spotify) is IMO about gathering additional info on the fly or looking for other tracks by that artist (not necessary similiar/matching tracks). Looking for covers and genre should IMO happen in the Track Properties. |
09765ba
to
c08c9cd
Compare
src/widget/wfindonmenu.h
Outdated
Discogs | ||
}; | ||
|
||
void openTheBrowser(const QString& serviceUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openInBrowser?
The serviceUrl should not be part of the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function can be moved to the anonymous namespace in the cpp file, because it does not use any members from "this"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make the URL private. But since I am using this method in WTrackMenu, this is a bit difficult. Or is it too easy and am I missing something?
I made private the Url in an another way. I defined a function in anonymous namespace that can return URL's according to chosen service, I am calling this function in openInBrowser, to return the urls from anonymous namespace. This time openInBrowser gets 2 arguments, which is the service and the query. But this time I needed to make "enum Service" public. So I am not sure which should be public and the other one private, is it okay to make Enum Service public?
src/widget/wfindonmenu.h
Outdated
void openTheBrowser(const QString& serviceUrl, | ||
const QString& query); | ||
|
||
void createAllServices(const Track& track); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"create all services"? I don't understand what this method is supposed to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, without a comment, it looks like this function created the whole service itself.
How about createSubmenusForAllWebLookups() or such.
By the way, in header we use /// comments that are picked up by doxygen and asigned to the following function which is able to automatically create an API decryption along with a class hierarchy.
src/widget/wfindonmenu.h
Outdated
const QString& query); | ||
|
||
private: | ||
void createService(QMenu* serviceMenu, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably do not "create" any service. Maybe you initialize some internal data structures.
The meaning of the serviceMenu
parameter is unclear for a method with an implicit this
of type QMenu
. Is it supposed to be the parent? Please name parameters according to their role.
Why is this a member functions instead of a hidden utility functions that builds and returns a QMenu*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serviceMenu is already a "service related submenu" the name should reveal this.
This function is also a candidate to be moved into the anonymous namespace.
Maybe the function can be called "populateWebLookUpQueries" ... It depends in which direction you will turn the interface.
src/widget/wfindonmenu.h
Outdated
void createAllServices(const Track& track); | ||
|
||
signals: | ||
void triggerBrowser( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between those 2 arguments with similar names and identical types? Very error prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe webLookupQuerry and porperty, or such.
You may also consider to use enums to release the menu from implementation details.
src/widget/wfindonmenu.h
Outdated
QWidget* parent = nullptr); | ||
~WFindOnMenu() override = default; | ||
|
||
enum class Services { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each value of this enum denotes a single service, not multiple. The type name is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as this enum is not needed by consumers of the menu make it private or hide it in the .cpp file in an anonymous namespace.
src/widget/wfindonmenu.h
Outdated
QString serviceTitleDisplay, | ||
Services serviceTitle); | ||
|
||
void addActionsArtist(Services serviceTitle, QString artist, QMenu* m_pService); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the naming of parameters:
- The
enum
variants do not denote a "title", they only refer to some external service. - The prefix
m_
is reserved for members QString
is supposed to be passed by const-ref
The term "find on" is unintuitive for a feature that actually performs a "web search". |
Please ensure that your code passes the CI checks, even if it is only a Draft PR. |
src/widget/wfindonmenu.cpp
Outdated
|
||
namespace { | ||
|
||
inline QString actionPrefixSuffixSeparator(const QString propertyType, const QString& text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline QString actionPrefixSuffixSeparator(const QString propertyType, const QString& text) { | |
inline QString actionPrefixSuffixSeparator(const QString& propertyType, const QString& text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "inline" should not be used to try to do better optimization than the compiler. The compiler will probably inline it anyway. Just don't care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should be named like what it does. Something like
composeActionText() or such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order is swapped compared to the "Find Similar" feature. I think I like the "Find Similar" order more.
src/widget/wfindonmenu.cpp
Outdated
} | ||
|
||
inline QString searchUrlSoundCloudArtist() { | ||
return QString("https://soundcloud.com/search/people?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this creates a new string on every call. Since Mixxx is no library we can do
QString searchUrlSoundCloudArtist = QStringLiteral("https://soundcloud.com/search/people?");
src/widget/wfindonmenu.cpp
Outdated
} | ||
} | ||
|
||
void WFindOnMenu::addActionsAlbum(Services serviceTitle, QString albumName, QMenu* m_pService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void WFindOnMenu::addActionsAlbum(Services serviceTitle, QString albumName, QMenu* m_pService) { | |
void WFindOnMenu::addActionsAlbum(Services serviceTitle, const QString& albumName, QMenu* m_pService) { |
src/widget/wfindonmenu.cpp
Outdated
const Track& track, | ||
QString serviceTitleDisplay, | ||
Services serviceTitle) { | ||
serviceMenu = new QMenu(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use p as prefix for pointers. In this case you can use partented_ptr, to express that it is owned by the Qt Object tree and it must net be deleted explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since WFindOnMenu is owned by the Q_OBJECT? So something like
pserviceMenu = new WFindOnMenu(this);
would solve this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto pServiceMenu = make_parented<QMenu>(this);
Since WFindOnMenu is inherited from QObject, it will delete all child QObjects once it is deleted, during the inherited QObject destructor.
parented_ptr makes sure that this is the case and will be still the case after refactoring.
Thanks for the feed backs, I will resolve the issues asap. I will be more careful with the naming, not gonna use inline functions and can do better search with discogs. Also CI too. |
src/widget/wfindonmenu.h
Outdated
QWidget* parent = nullptr); | ||
~WFindOnMenu() override = default; | ||
|
||
enum class Services { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as this enum is not needed by consumers of the menu make it private or hide it in the .cpp file in an anonymous namespace.
Nope. |
I am stopping my review activities at this point. @daschuer Please take over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found an improvement regarding the connections and some naming nit-picks.
2d5bf9c
to
8f309b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't really comment on this code otherwise as I have 0 experience with QWidgets or the mixxx library in general.
@daschuer is volunteering to address (or mentor @fatihemreyildiz to fix) any issues introduced by this PR. LGTM from me so I'll go ahead and merge. |
The intermediate commits should have been |
I'm sorry my mistake. I suppose squashing and force-pushing to |
I don't think so. Mistakes happen, don't worry. |
This is a draft PR.
As we mentioned on the Zulip #gsoc stream Web search Box feature topic, I wanted to add a small feature, that can help users to save time. A shortcut to find Artists on different services. For now I tried on SoundCloud.
I am facing with an issue. I couldn't fetch artist name on wtrackmenu. I have tried few things but they didn't work. At last, i tried something and it returns an error, which is signal and slot is not equal. I have looked at it, couldn't find a solution.
For the future of this feature, there can be one main row called as "Find on..." Then many rows could be there. Such as, "Find the artist on Discogs", "Find the track on LastFm" etc. In my estimation to make it bigger feature there should be something like search related track menu.