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

extended Dragndrop functionality #2684

Closed
wants to merge 18 commits into from
Closed

extended Dragndrop functionality #2684

wants to merge 18 commits into from

Conversation

Fice
Copy link
Contributor

@Fice Fice commented May 2, 2013

This PR let's the user reorder lists in Favourites and Playlists via drag&drop.
also it let's user drop items into the Favourites/playlists and FileManager lists.
Short demo video: http://www.youtube.com/watch?feature=player_embedded&v=rgMS-8xbEI0
Development thread: http://forum.xbmc.org/showthread.php?tid=161155

The skinner can either define a drag hint: a overlay CGUIControl, that will be visible between the two items, where the item would be placed
Or he can do nothing, and the dragged item will be moved immediately.

I'm not completely happy with how FixedLists and WrapLists behave right now, feels a little itchy (especially when you drag an item over the start/end of a wrap list), but everything should be functional at least.

Also: defining a DragHint in the skin, only works reliably as long as the focusedlayout does not overlay the normal layouts.

@pieh: Moved all the drag&drop related stuff out of CGUIWindow* with one exception: I didn't feel comfortable to just make any "C:/Movies" directory dropable, so the CGUIWindowFileManager is still responsible for setting the DroPolicy there. Hope that's ok?

P.S.: ios/os x/atv project files updated (but only compiled and testet on os x). Could use some input on other platforms, especially touch devices... only testet with mouse (don't hav a tablet yet).

@Fice
Copy link
Contributor Author

Fice commented May 5, 2013

@cptspiff everything updated... i didn't think github would remove all your comments completely though...

I wasn't happy with the name of the GetRealOffset() function I added, so i added a new commit with some renaming to clear things up

@jmarshallnz
Copy link
Contributor

I like the idea, and it's something we want, but this is a shitload of code so isn't going in in one chunk.

I suggest dividing it up into usable chunks. One might be just what you need for allowing items to drag within a list. Don't worry about the influence on whoever is supposed to "own" the list (e.g. favourites) at this point, just get the live re-ordering in. It might pay to also detail (in comments) exactly why you've chosen to implement it the way you have so that, if someone else thinks of a better way to do it we're not wasting time going through stuff you've already thought of :)

@Fice
Copy link
Contributor Author

Fice commented Jun 2, 2013

First I could drop some commits:
6d07b69 and 05d1319 are just meant as an example skin implementation, but they look rather ugly. I just added them, so anyone could test it.
276ef45 Favourites/Playlists and the Filebrowser usually have normal list views (not panels or anything), so in order to test the other containers is simply made the music db and the video db reorderable. That should definitely not go upstream.

Also, the first 3 commits could each be their own PR... they are mostly RFC but rather generic i guess?

The big whopper is a441669 that definitely could use some splitting.

My approach would now be to leave this pr as is (so people interested can go ahead and test the end result), while I create new PRs for each chunk, ok?

Btw: have you actually testet it? especially on a touch device?

@jmarshallnz
Copy link
Contributor

Sounds like a good plan. Have not tested it, no - am currently playing in a related area though so will try and do up a build for android at some point and have a play to see how it works on touch.

@Tolriq
Copy link
Contributor

Tolriq commented Jun 4, 2013

Just found this one that implement the Playlist.move that I proposed around the same time :)
Watching thread for merge to add this to JSON after merge :)

But should not the Playlist.move also implement a notification ?
Also what about the CApplicationMessenger::PlayListPlayerXXX things ? Should not a Move message and functions created ?
(This is how currently JSON modify playlists)

@Fice
Copy link
Contributor Author

Fice commented Jun 4, 2013

This PR won't go in as a whole, so I'm in the process of dissecting it into smaller, manageable pieces. So it will probably take a while, till everything is in.
I have just picked all the move parts and pushed it here: https://github.com/Fice/xbmc/commits/Move .
Feel free to use it, if you want

@jenkins4xbmc
Copy link
Contributor

Is this PR ment to be tested by jenkins?

@Fice
Copy link
Contributor Author

Fice commented Jun 16, 2013

Doubt it. This one will not go in as a whole but in smaller seperated PR's. The first one is #2826. Also it wouldn't compile on anything other that atv/ios and osx.

@Memphiz
Copy link
Member

Memphiz commented Jun 16, 2013

stop it fice ... that was an accident ... just ignore it ...

@MartijnKaijser
Copy link
Member

PR will be done in smaller chunks so closing this one

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

6 participants