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

Improve webplayer UX #623

Merged
merged 1 commit into from
Nov 10, 2019

Conversation

hinnerkoetting
Copy link
Contributor

Information and controls for the webplayer page was moved from server rendering to AJAX requests.
API endpoints were added to request status informations and send commands.
The Webpage can now be controlled without full page reloads so that it feels more responsive.

Also the number of calls to mopidy was reduced so that the page loads faster and mopidy
stability is improved.

    Information and controls for the webplayer page was moved from server rendering to AJAX requests.
    API endpoints were added to request status informations and send commands.
    The Webpage can now be controlled without full page reloads so that it feels more responsive.

    Also the number of calls to mopidy was reduced so that the page loads faster and mopidy
    stability is improved.
@@ -0,0 +1,175 @@
<?php
include ("lang/lang-en-UK.php");
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @hinnerkoetting this line makes me wonder if and how the ajax version should handle the interface language - which can be switched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MiczFlor I "solved" it by printing all localisations in the index.php, so that they can be accessed by javascript.
I guess it is not a perfect solution but should be good enough for now in my opinion.

@MiczFlor MiczFlor changed the base branch from master to develop November 10, 2019 15:19
@MiczFlor MiczFlor merged commit fec5922 into MiczFlor:develop Nov 10, 2019
@MiczFlor
Copy link
Owner

Hi @hinnerkoetting
gee, this is a lot of code :) thanks for the effort. Tackling the API was way overdue. I merged this to the develop branch. If you find the time, give it a test, I will do the same, soon. Currently work has me in a knot. And my code review is "reading only".

@hinnerkoetting
Copy link
Contributor Author

I really like this project, so I am happy that I could contribute some code.
I will probably test my changes again over the next couple of days. Thanks for merging it.

@MiczFlor
Copy link
Owner

Hi @hinnerkoetting
thanks for your warm words. Currently it's shortly after 7am, I am sitting in the Regio Bahn to Spandau with my laptop on my lap, looking at your work :) because I am so glad ajax is coming :)

if I switch from master to develop, the playlist disappears in the web UI. Thing is: on my linux machine, obviously, I am not running the webserver as root and there are many other changes which make it difficult to really test Phoniebox stuff on my laptop.
Could you possibly try a blank install with the install script and then switch to develop and see what's missing?
From what I sense in my commute, there is something missing to fill this part


            <div id="loadPlaylist">
                <table style="width: 100%; border-collapse: collapse; border-top: 1px solid #444;" id="playlistTable">
                </table>
            </div>

with the matching HTML.
Thanks again for making a start with this, I hope to be able to help pushing it over the finish line, merging to master.

@MiczFlor
Copy link
Owner

Hi @hinnerkoetting
another thing, I ran into: when clicking on the multiple play button in the web app, I get a "file not found". The reason being this line:

<a href='playPlaylist("PODCASTS", "true");' class='btn-panel-big btn-panel-col' title='Play (sub)folders'><i class='mdi mdi-animation-play-outline'></i></a>

where the href tries to open the URL - in my Firefox on apache2, but I assume it would do the same on the Pi.

The matching code in the Wep App UI is:


        <div class='panel-heading' id='headingIDPODCASTS'>
            <h4>
              <a href='playPlaylist("PODCASTS", "true");' class='btn-panel-big btn-panel-col' title='Play (sub)folders'><i class='mdi mdi-animation-play-outline'></i></a>
				  <span class='mb-0 playlist_headline' data-toggle='collapse' data-target='#collapseIDPODCASTS' aria-expanded='true' aria-controls='collapseIDPODCASTS' style='cursor:pointer;' title='Show contents'><i class='mdi mdi-folder-outline mdi-36px'></i> PODCASTS
              <i class='mdi mdi-arrow-down-drop-circle-outline'></i>             <span class='badge' title='Show folders'><i class='mdi mdi-folder-multiple'></i> 6</span>            <span class='badge' title='Show files'><i class='mdi mdi-library-music'></i> 0</span>
              </span>
            </h4>
        </div><!-- ./ .panel-heading -->

@hinnerkoetting
Copy link
Contributor Author

hinnerkoetting commented Nov 11, 2019

@MiczFlor: I will check it probably tomorrow evening. The second case seems to be that I forgot to change <a href='playPlaylist... to <a onclick='playPlaylist...

I am not sure what causes the first issue, the playlist should be filled in updatePlaylistData() in htdocs/inc.loadedPlaylist.php.
Could you please check if there is a javascript error or an error on the network tab? I would guess that there is a missing value in your playlist that I forgot to check it for null.

ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 17, 2019
- Split MPD response key-value pair at the position ": "
- All response keys in lower case
ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 17, 2019
- Rework MPD response processing, split key-value pairs at ": "
- Change all response keys to lower case for further processing

Introduce php-unit as testing framework
- Add composer as dependency manager
- Add phpunit and php-mock as dependencies
- Introduce namespace 'Jukebox' to enable mocking of build-in php functions e.g. "exec", "headers"
ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 17, 2019
- Rework "currentsong" response processing, split key-value pairs at ": "
- Change all response keys to lower case for further processing
- Change "playlist" command to "playlistinfo" to receive additional track info (pos, time, albumLength)
- Add singlequotes to directory shell arguments to support spaces in folders

Introduce php-unit as testing framework
- Add composer as dependency manager
- Add phpunit and php-mock as dependencies
- Introduce namespace 'Jukebox' to enable mocking of build-in php functions e.g. "exec", "headers"
ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 18, 2019
- Rework "currentsong" response processing, split key-value pairs at ": "
- Change all response keys to lower case for further processing
- Change "playlist" command to "playlistinfo" to receive additional track info (pos, time, albumLength)
- Add singlequotes to directory shell arguments to support spaces in folders
- Remove comma from playlist name if exists, to fix collapsable tracklist

Introduce php-unit as testing framework
- Add composer as dependency manager
- Add phpunit and php-mock as dependencies
- Introduce namespace 'Jukebox' to enable mocking of build-in php functions e.g. "exec", "headers"
@ctietze
Copy link
Contributor

ctietze commented Nov 18, 2019

Hi @MiczFlor , Hi @hinnerkoetting ,
thanks so far for the whole jukebox and the webplayer UX update!

I`m just building up a box and thought, I can help a little bit...
During testing of the current develop branch I just ran into the same issues as you described in the comments of #623 and #624

I already fixed some of them, but there are still some open issues

  • Podcasts has no cover displayed
  • Podcast playlist time shows "NaN"
  • Cannot play Playlist w/ subfolders

I pushed my first changes to an own branch [1], as it is still "work in progress".

Best regards
Clemens

[1] ctietze@7bfa17e

ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 18, 2019
- Rework "currentsong" response processing, split key-value pairs at ": "
- Change all response keys to lower case for further processing
- Change "playlist" command to "playlistinfo" to receive additional track info (pos, time, albumLength)
- Add singlequotes to directory shell arguments to support spaces in folders
- Remove comma from playlist name if exists, to fix collapsable tracklist

Introduce php-unit as testing framework
- Add composer as dependency manager
- Add phpunit and php-mock as dependencies
- Introduce namespace 'Jukebox' to enable mocking of build-in php functions e.g. "exec", "headers"
@MiczFlor
Copy link
Owner

Hi @ctietze
thanks for the input. Did I miss a pull request? Or did you currently push only to your fork?
Create a pull request, please - and I hope that @hinnerkoetting will join the bug squashing team :)

ctietze added a commit to ctietze/RPi-Jukebox-RFID that referenced this pull request Nov 20, 2019
- Rework "currentsong" response processing, split key-value pairs at ": "
- Change all response keys to lower case for further processing
- Change "playlist" command to "playlistinfo" to receive additional track info (pos, time, albumLength)
- Add singlequotes to directory shell arguments to support spaces in folders
- Remove comma from playlist name if exists, to fix collapsable tracklist
- Remove duplicate ":" from list argument
- Hide overalltime for e.g. podcasts

Introduce php-unit as testing framework
- Add composer as dependency manager
- Add phpunit and php-mock as dependencies
- Introduce namespace 'Jukebox' to enable mocking of build-in php functions e.g. "exec", "headers"
@ctietze
Copy link
Contributor

ctietze commented Nov 20, 2019

Hi @MiczFlor,
yeah I thought I could fix the " Playlist w/ subfolders / playlist recursive" issue before the PR, but I wasn`t able to fix it yesterday. I created the PR and will check back on the playlist recursive thing on the weekend! Except for one of you two was faster ;)

Best regards
Clemens

@hinnerkoetting
Copy link
Contributor Author

Sure, I will also have a look. Can you tell me which podcast you were listening to? That will probably make it easier to reproduce.

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

Successfully merging this pull request may close these issues.

None yet

3 participants