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

File list sorting by clicking on column headers #8041

Merged
merged 7 commits into from May 12, 2014

Conversation

Projects
None yet
10 participants
@PVince81
Member

PVince81 commented Apr 3, 2014

File list sorting by clicking on column headers.

This PR will contain file list sorting.

  • Sortable file list
  • Sortable trashbin
  • Sortable public page
  • Upload file while sorted must appear at the correct position
  • Sort indicator on headers (arrows?)
  • Clean up server-side comparators ?
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 3, 2014

Member
  • Client side unit test for sort/reload
  • Server side unit test for getFiles() with different sort values
Member

PVince81 commented Apr 3, 2014

  • Client side unit test for sort/reload
  • Server side unit test for getFiles() with different sort values
@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 3, 2014

Contributor

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4052/

Contributor

owncloud-bot commented Apr 3, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4052/

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 4, 2014

Contributor

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4060/

Contributor

owncloud-bot commented Apr 4, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4060/

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 9, 2014

Contributor

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4176/

Contributor

owncloud-bot commented Apr 9, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4176/

@PVince81 PVince81 changed the title from [WIP] File list sorting to File list sorting by clicking on column headers Apr 11, 2014

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 11, 2014

Member

Fixes #164 #6978

Also TODO:

  • make sorting by date the default in the trashbin
Member

PVince81 commented Apr 11, 2014

Fixes #164 #6978

Also TODO:

  • make sorting by date the default in the trashbin
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 11, 2014

Member

@jancborchardt please have a look, you might want to tweak the styles a bit.
I've added a mouse hover on the column headers and also the triangle icon.

Member

PVince81 commented Apr 11, 2014

@jancborchardt please have a look, you might want to tweak the styles a bit.
I've added a mouse hover on the column headers and also the triangle icon.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 11, 2014

Member

@DeepDiver1975 @icewind1991 I'm finished with this branch, but it is based off infinite scrolling, which means it will need rebase after infinite scroll is merged.

Member

PVince81 commented Apr 11, 2014

@DeepDiver1975 @icewind1991 I'm finished with this branch, but it is based off infinite scrolling, which means it will need rebase after infinite scroll is merged.

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 11, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4226/

Contributor

owncloud-bot commented Apr 11, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4226/

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 11, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4228/

Contributor

owncloud-bot commented Apr 11, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4228/

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81
Member

PVince81 commented Apr 11, 2014

@MTRichards

This comment has been minimized.

Show comment
Hide comment
@MTRichards

MTRichards Apr 11, 2014

Contributor

Awesome! :)

Contributor

MTRichards commented Apr 11, 2014

Awesome! :)

@icewind1991

This comment has been minimized.

Show comment
Hide comment
@icewind1991

icewind1991 Apr 14, 2014

Member

Works great, but why is sorting done server side instead of client side?

Member

icewind1991 commented Apr 14, 2014

Works great, but why is sorting done server side instead of client side?

@vgezer

This comment has been minimized.

Show comment
Hide comment
@vgezer

vgezer Apr 16, 2014

Contributor

Looks really nice :), but it seems some special characters in languages are not sorted properly. I think it was referred to: #7254

Contributor

vgezer commented Apr 16, 2014

Looks really nice :), but it seems some special characters in languages are not sorted properly. I think it was referred to: #7254

@jancborchardt jancborchardt added this to the ownCloud 7 milestone Apr 17, 2014

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 28, 2014

Member

@icewind1991 client side might be slower especially with many files. Also in the future we might want to have the sorting be done by the database engine, and also the paging (only loading a few results, not the whole list) on the server side as well.

@wakeup yes, you're right.

Member

PVince81 commented Apr 28, 2014

@icewind1991 client side might be slower especially with many files. Also in the future we might want to have the sorting be done by the database engine, and also the paging (only loading a few results, not the whole list) on the server side as well.

@wakeup yes, you're right.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 28, 2014

Member

Rebased. This is ready for review + testing.

Member

PVince81 commented Apr 28, 2014

Rebased. This is ready for review + testing.

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 28, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4482/

Contributor

owncloud-bot commented Apr 28, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4482/

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975
Member

DeepDiver1975 commented Apr 28, 2014

👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 28, 2014

Member

@DeepDiver1975 feel free to hijack this PR... oops too late you already did it. 😉
Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

Member

PVince81 commented Apr 28, 2014

@DeepDiver1975 feel free to hijack this PR... oops too late you already did it. 😉
Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 28, 2014

Member

@icewind1991 see my comment above about server side sorting. Are you ok with that approach ? Thanks.

Member

PVince81 commented Apr 28, 2014

@icewind1991 see my comment above about server side sorting. Are you ok with that approach ? Thanks.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Apr 28, 2014

Member

Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

All intentional here 😉

Member

DeepDiver1975 commented Apr 28, 2014

Just wanted to check whether it was intentional and you didn't submit to the wrong one ?

All intentional here 😉

@icewind1991

This comment has been minimized.

Show comment
Hide comment
@icewind1991

icewind1991 Apr 29, 2014

Member

If paging is done server side then sorting should obviously also be done server side but as long as the server sends the full filelist anyway, I would think client side sorting would always be faster since you don't need have an extra ajax request or query the database

Member

icewind1991 commented Apr 29, 2014

If paging is done server side then sorting should obviously also be done server side but as long as the server sends the full filelist anyway, I would think client side sorting would always be faster since you don't need have an extra ajax request or query the database

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 29, 2014

Member

I think the extra ajax request is acceptable when clicking on headers.
Some browsers like IE8 might be quite slow for sorting longer file lists (1000+) on the client-side initially before displaying them.

But one advantage of client-side sorting only would be to have consistent sort, always using an algo on the browser (locale/natural sort)

I would hope that the sorting would be done on the SQL level, but due to the natural sort algos we want to use (and putting the directories first) it is not yet possible.

Member

PVince81 commented Apr 29, 2014

I think the extra ajax request is acceptable when clicking on headers.
Some browsers like IE8 might be quite slow for sorting longer file lists (1000+) on the client-side initially before displaying them.

But one advantage of client-side sorting only would be to have consistent sort, always using an algo on the browser (locale/natural sort)

I would hope that the sorting would be done on the SQL level, but due to the natural sort algos we want to use (and putting the directories first) it is not yet possible.

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot Apr 29, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4517/

Contributor

owncloud-bot commented Apr 29, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4517/

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Apr 29, 2014

The inspection completed: 8 new issues, 16 updated code elements

scrutinizer-notifier commented Apr 29, 2014

The inspection completed: 8 new issues, 16 updated code elements

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Apr 30, 2014

Member

@icewind1991 besides IE8 there are also mobile browsers which might be slow as well to sort longer list of files.

Member

PVince81 commented Apr 30, 2014

@icewind1991 besides IE8 there are also mobile browsers which might be slow as well to sort longer list of files.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 2, 2014

Member

@icewind1991 I don't want to throw away the server side sort code just yet for the reasons mentionned above.
It is possible to make it work on the client side with 2-3 extra JS lines later if we ever want it later.
I might add this as part of the "sharing overview" task (on a separate branch) because it uses the OCS Share API that doesn't return sorted results.

Member

PVince81 commented May 2, 2014

@icewind1991 I don't want to throw away the server side sort code just yet for the reasons mentionned above.
It is possible to make it work on the client side with 2-3 extra JS lines later if we ever want it later.
I might add this as part of the "sharing overview" task (on a separate branch) because it uses the OCS Share API that doesn't return sorted results.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 2, 2014

Member

To clarify: the "sharing overview page" uses a subclass of FileList and displays a list of shared files.

Member

PVince81 commented May 2, 2014

To clarify: the "sharing overview page" uses a subclass of FileList and displays a list of shared files.

@Nowaker

This comment has been minimized.

Show comment
Hide comment
@Nowaker

Nowaker May 5, 2014

Any chance it gets merged into version 6?

Nowaker commented May 5, 2014

Any chance it gets merged into version 6?

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 5, 2014

Member

No. This is a complex change that will be part of owncloud 7. We only backport Bugfixes. But you can use the master daily builds as a preview.

Member

karlitschek commented May 5, 2014

No. This is a complex change that will be part of owncloud 7. We only backport Bugfixes. But you can use the master daily builds as a preview.

@karlitschek karlitschek closed this May 5, 2014

@DeepDiver1975 DeepDiver1975 reopened this May 5, 2014

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 5, 2014

Member

Ups. Wrong button. Thanks @DeepDiver1975 :-)

Member

karlitschek commented May 5, 2014

Ups. Wrong button. Thanks @DeepDiver1975 :-)

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot May 5, 2014

Contributor

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4556/

Contributor

owncloud-bot commented May 5, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4556/

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975
Member

DeepDiver1975 commented May 5, 2014

@karlitschek np 😉

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 5, 2014

Member

@owncloud-bot retest this please

Member

DeepDiver1975 commented May 5, 2014

@owncloud-bot retest this please

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot May 5, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4557/

Contributor

owncloud-bot commented May 5, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4557/

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 7, 2014

Member

As discussed with @karlitschek, server-side sort is the way to go for the files app.

Member

PVince81 commented May 7, 2014

As discussed with @karlitschek, server-side sort is the way to go for the files app.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2014

Member

@icewind1991 can we merge this now ?

Member

PVince81 commented May 10, 2014

@icewind1991 can we merge this now ?

@icewind1991

This comment has been minimized.

Show comment
Hide comment
@icewind1991

icewind1991 May 12, 2014

Member

Yes 👍

Member

icewind1991 commented May 12, 2014

Yes 👍

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2014

Member

@jancborchardt I didn't receive any feedback about the design so I will merge this first.
The design / look can be tweaked in separate PRs.

Member

PVince81 commented May 12, 2014

@jancborchardt I didn't receive any feedback about the design so I will merge this first.
The design / look can be tweaked in separate PRs.

PVince81 added a commit that referenced this pull request May 12, 2014

Merge pull request #8041 from owncloud/files-sortcolumns
File list sorting by clicking on column headers

@PVince81 PVince81 merged commit 9a9665f into master May 12, 2014

1 check passed

default Merged build finished.
Details

@PVince81 PVince81 deleted the files-sortcolumns branch May 12, 2014

@jbtbnl jbtbnl referenced this pull request May 12, 2014

Closed

Design review of file sorting #8551

4 of 4 tasks complete
@jancborchardt

This comment has been minimized.

Show comment
Hide comment
@jancborchardt

jancborchardt May 12, 2014

Member

@PVince81 better always mention @owncloud/designers instead of just me alone as otherwise I’ll drown in issue notifications. ;)

Anyway, see @jbtbnl’s review at #8551

Member

jancborchardt commented May 12, 2014

@PVince81 better always mention @owncloud/designers instead of just me alone as otherwise I’ll drown in issue notifications. ;)

Anyway, see @jbtbnl’s review at #8551

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