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

Qobuz Player integration #438

Closed
bors-ltd opened this issue May 19, 2018 · 16 comments
Closed

Qobuz Player integration #438

bors-ltd opened this issue May 19, 2018 · 16 comments
Assignees

Comments

@bors-ltd
Copy link

I was eager to stumble upon Nuvola and find a way to pause this web player from the keyboard multimedia keys. Because, as usual, Qobuz won't provide a Linux application.

This is my first time today with both Nuvola and writing a service integration, so please bear with me.

bors-ltd added a commit to tiliado/nuvola-app-qobuz that referenced this issue May 19, 2018
First version, 80% complete.

Only seeking and volume changing are missing but I basically never use
them.

No fancy feature from latest API versions.

Issue: tiliado/nuvolaplayer#438
bors-ltd added a commit to tiliado/nuvola-app-qobuz that referenced this issue May 19, 2018
First version, 80% complete.

Only seeking and volume changing are missing but I basically never use
them.

No fancy feature from latest API versions.

- Author: Hervé Cauwelier <[email protected]>
- Reviewed by: FIXME <FIXME>
- Issue: tiliado/nuvolaplayer#438
@jiri-janousek
Copy link
Member

Hello @bors-ltd. Thanks for your work. Since you have a working script, you can decide on the level of cooperation:

  1. You can use it for your own purposes. There are no requirements from my side.
  2. You can have it published in the master (devel/unstable) branch of the Nuvola Apps flatpak repository. For that, your script has to comply with a few guidelines and be maintained under the Tiliado organization, but you will still have direct commit access to the GitHub repo. Scripts from the devel repo are not announced to the public though. You'll be invited to the Tiliado organization on GitHub and receive a Tiliado developer account on Tiliado.eu so that you can use all Nuvola flatpaks without paying.
  3. You can have it published in the stable branch of the Nuvola Apps flatpak repository and later maybe in Ubuntu's snap store. For that, your script must be kept up-to-date with Nuvola guidelines and features.

What's your choice?

@bors-ltd
Copy link
Author

bors-ltd commented May 21, 2018

Let me put that another way:

I'll use it on a daily basis (I work with music), and I hope my work will be useful to others.

So yes I'll maintain the script and do the remaining work for it to be published in the stable branch. It should be details only, I tried to follow the docs, and the code is already checked.

The biggest issue for me is how you feel about not implementing volume change (I want my volume keys to act at the system level, not a single app), and seeking.

I'd also like your opinion on the querySelector queries. I wanted to hit an ID before searching for classes in the subtree, but maybe it ended up making too complicated code. Maybe I should not worry about performance and just write document.querySelector('.pct-player-play').

I also wanted to pause the player when headphones are unplugged but it doesn't seem to be documented. Yet?

@jiri-janousek
Copy link
Member

Great, so the best option is to go for stable repo - that way your work will be most useful for others :-)

The biggest issue for me is how you feel about not implementing volume change (I want my volume keys to act at the system level, not a single app), and seeking.

Fot the stable channel, the integration of a volume bar is mandatory if there is any. However, it has nothing to do with volume keys, Nuvola does not handle them. It merely exposes the functionality for other clients, e.g. Media Player GNOME Shell extension provides a volume bar: Media Player GNOME Shell extension

The integration of progress bar is mandatory too unless there are relevant reasons not to do so. For example, Brain.fm and Focus@Will don't provide track length and position, so the progress bar cannot be integrated.

"I hardly use it ever" is ok if you create script only for you, but not ok if you create it also for others - they might want to use it. There were users that asked specifically for the volume and progress bar integration.

I'd also like your opinion on the querySelector queries. I wanted to hit an ID before searching for classes in the subtree, but maybe it ended up making too complicated code. Maybe I should not worry about performance and just write document.querySelector('.pct-player-play').

I don't have a hard opinon about which way is better, they are both fine. Quering for bottomPlayerContainer and then for subtree elements looks more readable though.

I also wanted to pause the player when headphones are unplugged but it doesn't seem to be documented. Yet?

Indeed, this feature is not documented yet (#423). You can look at the blog post.

If you would like to proceed further:

  • I've invited you to Nuvola Maintainers team. After you accept the invitation, could you transfer your repository to that team? You will still be the maintainer of the repository with a complete write access. However, in case you leave the project, somebody else can carry on.
  • Please create a Tiliado account and tell me your username. Your account will be then upgraded to developer account and you will gain free access to flatpak builds of Nuvola 4.x including all premium features.
  • Subscribe to the nuvola-player-devel mailing list as important announcements are posted there. This mailing list has low traffic so don't be afraid that your mailbox will be full of spam.
  • Take a look at Service Integration Guidelines: Rules and guidelines you should follow. Please make sure you are familiar with the process how releases are made. If you have any questions about the guidelines or suggestions to improve them, don't hesitate to ask here.
  • Make yourself familiar with Nuvola ADK and try to build & run your script. If there are any issues, don't hesitate to ask here.
  • Make sure that your script works with the latest release of Nuvola ADK and tell me whether it does. I won't publish your script until I receive an explicit confirmation from you.
  • New scripts should be state of the art at the time of the first release. They should take advantage of the latest available Nuvola's features and APIs to provide the best system integration possible. Could you take a look at progress bar and volume management integration (both added in Nuvola 4.5)?
  • Could you look at the ticket #424 whether some points apply to your script?

@bors-ltd
Copy link
Author

Good news, I misinterpreted the volume feature, and that shell extension is cool, I already adopted it. It will also be great to test my script.

I'll do the extra work and review the procedure, but probably next week-end now.

@bors-ltd
Copy link
Author

Reporting my duties:

  • I transferred the repository to Nuvola Maintainers and I could check I can push to it.
  • I created the account "bors-ltd" on Tiliado (the same as Github for clarity).
  • I subscribed to the nuvola-player-devel mailing list.
  • I actually already read Service Integration Guidelines but sure I'll keep it open when I develop.
  • My script is already functional, that's why I submitted it. ;-)
  • The template I used was for API 4.11, and I think I always used the CEF engine, I could never right-click on the app to open the inspector.
  • Alas, I can't seek in the song, I added the event, I send coordinates in the range [0..1] but it won't seek. As I can't seek myself in the webview, maybe it's a older Chromium or Qobuz incompatibility. I checked I could seek in Chromium 66.0.3359.181 and Firefox, and no longer if I delete the element from the DOM, so maybe a future version of CEF will fix this. I committed the code anyway.
  • I added volume management too. I also added URL filtering last week.
  • I think I'm safe for ticket Modernize remaining scripts [Due 2018-07-01] #424 since I started this project from stratch using the latest ADK last week. I added a snapshot and there is no rating on Qobuz. And they got rid of Flash a long time ago.
  • I could verify the headphone detection works like a charm, with nothing to develop on my side, great job!

Side question: what would the version number be for the first release? It was 1.1 in the template, I downgraded it to 1.0 and now I added features.

@jiri-janousek
Copy link
Member

I created the account "bors-ltd" on Tiliado (the same as Github for clarity).

I upgraded you Tiliado account to Tiliado developer account. You can now use it to activate the genuine flatpak builds of Nuvola.

I think I always used the CEF engine, I could never right-click on the app to open the inspector.

I think Chromium was triggered with Feature[MSE]', which is not supported by the WebKitGTK backend. We now use Chromium[65] Codec[MP3] Feature[MSE]` because these features are builtin for Chromium. If porters want to use WebKitGTK backend, they need test whether MSE is really required or not - we don't know.

Alas, I can't seek in the song, I added the event, I send coordinates in the range [0..1] but it won't seek. As I can't seek myself in the webview, maybe it's a older Chromium or Qobuz incompatibility.

Could you install the devel branch of the ADK and try with VALACEF_DEFAULT_RENDERING_MODE=offscreen? E.g. flatpak install nuvola eu.tiliado.NuvolaAdk//master and VALACEF_DEFAULT_RENDERING_MODE=offscreen flatpak run eu.tiliado.NuvolaAdk//master -D + add filesystem flags?

@jiri-janousek
Copy link
Member

Side question: what would the version number be for the first release? It was 1.1 in the template, I downgraded it to 1.0 and now I added features.

The first release will be 1.1 because there already is version 1.0 in metadata from the beginning. It then gets incremented to 1.1 when the release commit is made.

@jiri-janousek
Copy link
Member

You can now use VALACEF_DEFAULT_RENDERING_MODE=offscreen with stable builds as well.

@bors-ltd
Copy link
Author

bors-ltd commented Jun 2, 2018

Is the chromium window supposed to show in offscreen mode? Anyway, I couldn't seek, including trying manually by dragging the handle myself in the Qobuz app.

I know the flag was taken into account:

Runner: [INFO Δ000270us CefGtk] WebView.vala:74: CEF rendering mode: CEF_GTK_RENDERING_MODE_OFFSCREEN

In the meantime, I tried with CEF itself and its cefsimple demo app, and I couldn't seek either. And since it was version 67.0.3396.30, the same than my regular chromium browser, I don't think we can wait for an upstream bugfix. Could it be a missing flag or feature in the way CEF is built? Or the Qobuz app poorly designed, of course.

Do we release anyway with a known bug?

@bors-ltd
Copy link
Author

bors-ltd commented Jun 2, 2018

I tried to remove CSS classes from the seek bar to remove anything fancy, but even if CEF is showing the native input handle (it's a <input type="range"> and I removed the -webkit-appearance: none; property), and even highlight it on mouse hover, I can't drag it.

I also wrote the snippet below that quite reproduces the seek bar and clicking on the input or dragging the handle works just fine. :-/

My conclusion is that something in the Qobuz app is eating up the event.

<html>
<body>
<div style="width: 100%; height: 55px">
<div style=" 
    position: absolute;
    left: 130px;
    top: 27px;
    width: 100%;
"> 
<progress step="0.01" min="0" max="287" value="0" style="
    -webkit-appearance: none;
    display: inline-block;
    width: 100%;
    height: 5px;
    cursor: pointer;
    background: #0091c5;
    position: absolute;
    top: 0;
    left: 0;
    z-index: 5;
"></progress>
<input type="range" step="0.01" min="0" max="287" value="0" style="
    -webkit-appearance: none;
    background: transparent;
    display: block;
    width: 100%;
    height: 14px;
    cursor: pointer;
    margin: 0;
    padding: 0;
    position: relative;
    top: -5px;
    z-index: 110;
">
<p></p>
</div>
<div> 
<script>
(function showInput() {
  try {
    var value = document.querySelector('input').value
    document.querySelector('progress').value = value
    document.querySelector('p').innerText = value
  } catch (e) {}
  setTimeout(showInput, 1000)
})()
</script>
</body>
</html>

@jiri-janousek
Copy link
Member

Is the chromium window supposed to show in offscreen mode?

No, Chromium window is not supposed to show. Chromium should render to a pixbuf that is then rendered into Nuvola window.

Anyway, I couldn't seek, including trying manually by dragging the handle myself in the Qobuz app.

I'll try to look at it.

Could it be a missing flag or feature in the way CEF is built? Or the Qobuz app poorly designed, of course.

No idea.

Do we release anyway with a known bug?

I think the ap is still useful without seeking.

@jiri-janousek jiri-janousek self-assigned this Jun 2, 2018
@bors-ltd
Copy link
Author

bors-ltd commented Jun 5, 2018

No, Chromium window is not supposed to show. Chromium should render to a pixbuf that is then rendered into Nuvola window.

So I meant the Nuvola window? Anyway, I couldn't spot the difference between the two modes without checking the logs.

@jiri-janousek
Copy link
Member

So I meant the Nuvola window? Anyway, I couldn't spot the difference between the two modes without checking the logs.

In the windowed rendering mode (default), Chromium renders to its window and Chromium window is embedded into Nuvola window. In the offscreen rendering mode, there is no Chromium window, Chromium renders to an offscreen pixel buffer and then Nuvola renders the data to its window. There are a few differences under the hood: https://github.com/tiliado/valacef/issues/11#issuecomment-391983463

jiri-janousek added a commit that referenced this issue Jun 10, 2018
@jiri-janousek
Copy link
Member

  • I can confirm the seek issue. So far no luck with that but I didn't have enough time to look at it closer.
  • I created a flatpak for the master branch of the flatpak repo: flatpak install nuvola eu.tiliado.NuvolaAppQobuz.
  • I may release it to the stable branch at the weekend. I think the script is useful even without seeking.

@jiri-janousek
Copy link
Member

jiri-janousek commented Jun 13, 2018

The seeking issue reticketed as tiliado/nuvola-app-qobuz#1

@jiri-janousek
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants