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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] New playlist functionality (Final) #266

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
@vgezer
Contributor

vgezer commented Aug 10, 2014

馃毀 Work in progress!

Ready:

  • Sidebar with playlists
  • Hardcoded playlists
  • Hardcoded song display
  • Playlist - Database connection
  • Create basic playlist functionalities (create, edit, delete playlist)
  • Play the next song in the playlist after the current song
  • Add songs by drag&drop
  • Remove songs from playlists

Playlists:

  • Repeat playlist songs
  • Shuffle playlist songs

Working on:

  • Design improvements

To do:

  • Unit tests
  • Documentation

Issues:

Apps menu is not shown: change z-index
Shuffle & repeat buttons are not shown, check css
Mobile view

Playlists may:

  • be sorted by dragging

... and will be updated

@vgezer vgezer self-assigned this Aug 10, 2014

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer Aug 10, 2014

Contributor

@MorrisJobke Here is the squashed without conflict playlist PR. I think we can close the other one as everything works here.

Contributor

vgezer commented Aug 10, 2014

@MorrisJobke Here is the squashed without conflict playlist PR. I think we can close the other one as everything works here.

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer Aug 11, 2014

Contributor

Issues:

Apps menu is not shown: change z-index
Shuffle & repeat buttons are not shown, check css

Contributor

vgezer commented Aug 11, 2014

Issues:

Apps menu is not shown: change z-index
Shuffle & repeat buttons are not shown, check css

@@ -20,6 +20,11 @@
#app-view {
height: 100%;
top: 0;

This comment has been minimized.

@MorrisJobke

MorrisJobke Aug 11, 2014

Member

This will cause the mobile responsiveness to fail.

@MorrisJobke

MorrisJobke Aug 11, 2014

Member

This will cause the mobile responsiveness to fail.

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Aug 12, 2014

Member

After you're fine with my changes I would like to pull the playlist controller into the master branch. Then we have a design-only PR and can tweak it. Are you fine with that?

Member

MorrisJobke commented Aug 12, 2014

After you're fine with my changes I would like to pull the playlist controller into the master branch. Then we have a design-only PR and can tweak it. Are you fine with that?

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Aug 31, 2014

Member

I need to rebase it and test again

Member

MorrisJobke commented Aug 31, 2014

I need to rebase it and test again

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Sep 2, 2014

Member

@wakeup Can I ask you to rebase this on master (but be careful the player has changed to aurora.js)

Member

MorrisJobke commented Sep 2, 2014

@wakeup Can I ask you to rebase this on master (but be careful the player has changed to aurora.js)

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer Sep 2, 2014

Contributor

@MorrisJobke Yes sure. I will be doing it in the evening.

Contributor

vgezer commented Sep 2, 2014

@MorrisJobke Yes sure. I will be doing it in the evening.

vgezer and others added some commits Apr 30, 2014

Refactoring for production use
* removal of unused CSS classes
* drop debug output
* drop PlaylistFactory
* renamings of playlist controller functions
* don't reload playlist information after change(via REST-API) - just apply the change to the local object
* better variable namings
* coding style
* pass in objects instead of just the id and then searching for the object again
@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer Sep 3, 2014

Contributor

@MorrisJobke I rebased, but I get this error when choosing a playlist:

<b>Fatal error</b>:  Call to a member function getById() on a non-object in <b>/var/www/owncloud/apps/music/db/track.php</b> on line <b>95</b><br />
Contributor

vgezer commented Sep 3, 2014

@MorrisJobke I rebased, but I get this error when choosing a playlist:

<b>Fatal error</b>:  Call to a member function getById() on a non-object in <b>/var/www/owncloud/apps/music/db/track.php</b> on line <b>95</b><br />
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Sep 3, 2014

The inspection completed: 3 new issues, 2 updated code elements

The inspection completed: 3 new issues, 2 updated code elements

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt Dec 1, 2015

Member

@MorrisJobke @vgezer so what鈥檚 the status on this?

Member

jancborchardt commented Dec 1, 2015

@MorrisJobke @vgezer so what鈥檚 the status on this?

@remiheens

This comment has been minimized.

Show comment
Hide comment
@remiheens

remiheens Apr 28, 2016

So, do you have an idea about the release date ?

remiheens commented Apr 28, 2016

So, do you have an idea about the release date ?

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt Apr 29, 2016

Member

@vgezer can you give any indication what you are up to? ;) Or @MorrisJobke what do you think is the state of this?

(@remiheens no idea basically 鈥 are you up to contribute maybe? :)

Member

jancborchardt commented Apr 29, 2016

@vgezer can you give any indication what you are up to? ;) Or @MorrisJobke what do you think is the state of this?

(@remiheens no idea basically 鈥 are you up to contribute maybe? :)

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer May 1, 2016

Contributor

I think I will be contributing to ownCloud in general as well as this repo from July again :). I haven't tested this for OC9, but @remiheens can give it a try and let us know :)

Contributor

vgezer commented May 1, 2016

I think I will be contributing to ownCloud in general as well as this repo from July again :). I haven't tested this for OC9, but @remiheens can give it a try and let us know :)

@remiheens

This comment has been minimized.

Show comment
Hide comment
@remiheens

remiheens May 2, 2016

Yes, I will test your branch on OC9.

Yes, I will test your branch on OC9.

@remiheens

This comment has been minimized.

Show comment
Hide comment
@remiheens

remiheens May 2, 2016

Ok, I've tested this PR.

Playing music still works, Create playlist in sidebar works but

  • Playlist icons on hover don't exist.
  • Playlist view don't work (api/playlists/2/add return a 200 - {"name":"TEST","trackIds":[1],"id":2} - table music_playlist_tracks have rows)
  • 2 ajax call were launch when you click on playlist in sidebar

See my log :

[
    {
        "app": "PHP",
        "level": 3,
        "message": "Declaration of OCA\\Music\\Db\\PlaylistMapper::delete() should be compatible with OCP\\AppFramework\\Db\\Mapper::delete(OCP\\AppFramework\\Db\\Entity $entity) at /var/www/owncloud/apps/music/db/playlistmapper.php#20",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Missing argument 2 for OCA\\Music\\Db\\Track::toCollection(), called in /var/www/owncloud/apps/music/controller/playlistapicontroller.php on line 126 and defined at /var/www/owncloud/apps/music/db/track.php#94",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Undefined variable: userFolder at /var/www/owncloud/apps/music/db/track.php#95",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Call to a member function getById() on null at /var/www/owncloud/apps/music/db/track.php#95",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Declaration of OCA\\Music\\Db\\PlaylistMapper::delete() should be compatible with OCP\\AppFramework\\Db\\Mapper::delete(OCP\\AppFramework\\Db\\Entity $entity) at /var/www/owncloud/apps/music/db/playlistmapper.php#20",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "4CNoEX5lPKKRJc5bvaiJ",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists",
        "user": "Remi"
    }
]

To install this branch, i've checkout on master then merge branch playlist-last on it and composer/bower update.

If I've time, I will debugging OC9 compatibility.

remiheens commented May 2, 2016

Ok, I've tested this PR.

Playing music still works, Create playlist in sidebar works but

  • Playlist icons on hover don't exist.
  • Playlist view don't work (api/playlists/2/add return a 200 - {"name":"TEST","trackIds":[1],"id":2} - table music_playlist_tracks have rows)
  • 2 ajax call were launch when you click on playlist in sidebar

See my log :

[
    {
        "app": "PHP",
        "level": 3,
        "message": "Declaration of OCA\\Music\\Db\\PlaylistMapper::delete() should be compatible with OCP\\AppFramework\\Db\\Mapper::delete(OCP\\AppFramework\\Db\\Entity $entity) at /var/www/owncloud/apps/music/db/playlistmapper.php#20",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Missing argument 2 for OCA\\Music\\Db\\Track::toCollection(), called in /var/www/owncloud/apps/music/controller/playlistapicontroller.php on line 126 and defined at /var/www/owncloud/apps/music/db/track.php#94",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Undefined variable: userFolder at /var/www/owncloud/apps/music/db/track.php#95",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Call to a member function getById() on null at /var/www/owncloud/apps/music/db/track.php#95",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "6uDrQPIzG/60kkkODNZ9",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists/2",
        "user": "Remi"
    },
    {
        "app": "PHP",
        "level": 3,
        "message": "Declaration of OCA\\Music\\Db\\PlaylistMapper::delete() should be compatible with OCP\\AppFramework\\Db\\Mapper::delete(OCP\\AppFramework\\Db\\Entity $entity) at /var/www/owncloud/apps/music/db/playlistmapper.php#20",
        "method": "GET",
        "remoteAddr": "89.91.146.13",
        "reqId": "4CNoEX5lPKKRJc5bvaiJ",
        "time": "2016-05-02T15:46:31+00:00",
        "url": "/index.php/apps/music/api/playlists",
        "user": "Remi"
    }
]

To install this branch, i've checkout on master then merge branch playlist-last on it and composer/bower update.

If I've time, I will debugging OC9 compatibility.

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer May 3, 2016

Contributor

Thanks for working on it!

Contributor

vgezer commented May 3, 2016

Thanks for working on it!

@remiheens

This comment has been minimized.

Show comment
Hide comment
@remiheens

remiheens May 3, 2016

Ok, I fixed the functionality for OC9. Everything works well.
How do you want me to send you the changes?

Ok, I fixed the functionality for OC9. Everything works well.
How do you want me to send you the changes?

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt May 3, 2016

Member

I guess best would be a pull request which supersedes this one? Because that will have @vgezer鈥檚, @MorrisJobke鈥檚 and your changes. :)

Member

jancborchardt commented May 3, 2016

I guess best would be a pull request which supersedes this one? Because that will have @vgezer鈥檚, @MorrisJobke鈥檚 and your changes. :)

@remiheens

This comment has been minimized.

Show comment
Hide comment
@remiheens

remiheens May 3, 2016

A pull request on this branch ? (playlist-last)

Sorry, but this is my first public contribution.
I have to fork owncloud/music repository, checkout on playlist-last, apply changes, push my forked playlist-last, and create a PR for my own playlist-last branch ?

remiheens commented May 3, 2016

A pull request on this branch ? (playlist-last)

Sorry, but this is my first public contribution.
I have to fork owncloud/music repository, checkout on playlist-last, apply changes, push my forked playlist-last, and create a PR for my own playlist-last branch ?

@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt May 3, 2016

Member

Yep, exactly. And create the PR against the master branch (but base it off this branch). Then the changes by @vgezer and @MorrisJobke will automatically be in the PR since you based it off playlist-last. :)

Also, welcome to ownCloud! :)

Member

jancborchardt commented May 3, 2016

Yep, exactly. And create the PR against the master branch (but base it off this branch). Then the changes by @vgezer and @MorrisJobke will automatically be in the PR since you based it off playlist-last. :)

Also, welcome to ownCloud! :)

@paulijar paulijar referenced this pull request Dec 1, 2016

Merged

Another attempt to finalize the playlist functionality #555

8 of 8 tasks complete
@paulijar

This comment has been minimized.

Show comment
Hide comment
@paulijar

paulijar Dec 1, 2016

Collaborator

Superseded by #555

Collaborator

paulijar commented Dec 1, 2016

Superseded by #555

@paulijar paulijar closed this Dec 1, 2016

@paulijar paulijar deleted the playlist-last branch Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment