Skip to content
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: Implementing the factory pattern #4836

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

fatihemreyildiz
Copy link
Contributor

Hey everyone, this PR is the continue of #4772

This feature is called Find On Web. Which is located under the Metadata menu, with a sub menus of different online databases that can help users to look up track properties on online databases. It will populate the fav browser of the user with the given service and query. It basically looks like this:
FindOn

This feature is aimed for the users who accordingly looks additional info about the tracks. There are few queries with the chosen track. We can lookup for only for the artist, only for the track title (It is good for to look for remixes), track title + artist (make sure we find the correct song), artist + album (To find the other songs in that album). If has no property, the menu is grayed out.

This can be also helpful for later for to find suggested songs on the online services as discussed on the Zulip channel.

Factory Method used for this feature, so for later on another online databases (services) can be added easily by just changing the URL of the searches.

@fatihemreyildiz
Copy link
Contributor Author

This PR is the original PR of find on web feature (and also continued of the #4772). What do you think? Any ideas and feedbacks?

Should It be merged in two steps or in one step?

@daschuer
Copy link
Member

daschuer commented Jul 7, 2022

@fatihemreyildiz The previous PR is now merged, can you rebase this on main and force push, that we see a clean diff?
Thank you.

@fatihemreyildiz
Copy link
Contributor Author

Done 👍

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you. This way it is fairly easy to add new destinations.

@daschuer daschuer requested a review from Swiftb0y July 8, 2022 17:50
@daschuer
Copy link
Member

daschuer commented Jul 8, 2022

@Swiftb0y Can you have a final look?

src/widget/findonwebmenufactory.h Outdated Show resolved Hide resolved
src/widget/wfindonwebmenu.h Outdated Show resolved Hide resolved
src/widget/wfindonwebmenu.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for addressing my review comments so quickly.

wfindonwebmenu: includes changes.

wfindonwebmenu: service url composer added.

findonwebmenufactory: class moved in namespace.

wfindonwebmenu: addActionServiceMenu add, for DRY

wfindonwebmenu: url composer added to each service
@fatihemreyildiz
Copy link
Contributor Author

I have squashed all the commits.

Thank you for your feedback and help 🐙 @Swiftb0y @daschuer

@daschuer
Copy link
Member

Two approvals and squashed = ready to merge.
Thank you @fatihemreyildiz and @Swiftb0y

@daschuer daschuer merged commit e1ee933 into mixxxdj:main Jul 15, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants