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

Sorts favourite files first in list #302

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

ryanwr
Copy link
Contributor

@ryanwr ryanwr commented Oct 2, 2016

Shows files marked as favourites first, closes issue #107

@AndyScherzinger
Copy link
Member

@tobiasKaminsky can you (also) have a look? Code looks good to me. To be discussed if (as implemented in this PR by @ryanwr) there should be 2 chained sortings or if the sorting for the favorites should be incorporated in the 3 existing sorting implementations. That way you don't have to do a second sorting which makes the sorting implementations more complex but would save us 1 complete/additinal loop especially when it comes to very large file listings.

@tobiasKaminsky
Copy link
Member

I doubt that there is a real performance loss as there are very very few favorites files I guess.
So I would vote for keeping it this way.
Or is someone eager doing some performance tests? ;-)

@AndyScherzinger
Copy link
Member

Then I am fine doing it like this 👍
I'll will test it again (probably tomorrow) for all 3 sorting-options and if it works correctly will then merge it.

@AndyScherzinger
Copy link
Member

Test were successful - so merging to master.

Thanks @ryanwr for your contribution! 🎉 This will be part of the next stable release 1.4.0

@jancborchardt
Copy link
Member

@ryanwr very cool contribution! :) Added you to the Nextcloud organization and Android team :)

@ryanwr
Copy link
Contributor Author

ryanwr commented Oct 19, 2016

Thanks! Happy to help

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