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

Added sorting by name, date or size ascending/descending #624

Closed
wants to merge 12 commits into from

Conversation

Projects
None yet
7 participants
@tobiasKaminsky
Copy link
Contributor

commented Sep 14, 2014

Hello,
I have added the possibility to sort files/folders by name, date or size.
The option is stored temporary, so it is obeyed if the directory is changed.

Filename sorting fixes also #379
Sorting by size: Folder are sorted by name, as there is no information about the number of files containing the directory.

Sorting is, like dolphin or windows explorer, separated between folder and files.

TODO: Translate to other languages.

@davivel

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

This is something to discuss with @jancborchardt . An screenshot would be appreciated.

Forget about translations, we only define English in development. Translations are done in and pulled from Transifex , synchronized daily with the branch 'develop'.

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

I have made three screenshots:

  1. enhanced file sorting
  2. "Sorting" option after clicking on the three points
  3. available sorting options:
  • Filename - ascending
  • Filename - descending
  • Date - ascending
  • Date - descending
  • Filesize - ascending
  • Filesize - descending

screenshot_2014-09-15-09-43-15
screenshot_2014-09-15-09-43-23
screenshot_2014-09-15-09-43-30

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

Nice, this look good!

@tobiasKaminsky one thing: The words »ascending« and »descending« are quite confusing. Instead use the more understandable labels:

  • Name, A-Z
  • Name, Z-A
  • Date, newest first
  • Date, oldest first
  • Size, biggest first
  • Size, smallest first
@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

tobiasKaminsky added some commits Sep 15, 2014

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

I have added a small change:
Now the folders are always on top and get sorted, then the files are displayed.
Before that it occured that the folders were on the bottom as the list was only reversed.
Now the behaviour is exactly the same like in dolphin (KDE file browser).

One thing left: When "Sorting by size" all folders stay in A-Z order as there is no information how big/many files they have. One could iterate over every folder and sum up its content, but this is maybe too much, or?

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

@davivel don’t we have the folder size info by now? The web client displays it with no problems. cc @icewind1991 on how we do that there.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

@tobiasKaminsky in the list of the sorting options – let’s actually cut it down to 3:

  • A - Z
  • Newest - Oldest
  • Biggest - Smallest

The reverse orders are of little use and clutter up the list.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

And if we don’t have the folder size info for now, we can start out with the »A - Z« and »Newest - Oldest« sorting options.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

By the way, they should be styled as radio buttons so it’s obvious which one is currently active.

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

  • Radio buttons are done (I will add it later when the other questions are solved)
  • I personally like the possibility to reverse the sorting order as otherwise one has to scroll down the complete list. Currently I am thinking about a global switch, which is located on the bottom (or top) of the dialog called "reverse", which is a toggle button. What do you think?
@MTRichards

This comment has been minimized.

Copy link

commented Sep 15, 2014

Great stuff here.

@jancborchardt the issue with folder size was one of performance, if I remember. I don't know if we can get folder size directly from the server, or if we have to create it locally - if I remember, the issue was that we had to calculate it, which takes time with each sub folder query of the server - and gets worse the bigger the tree.

@davivel Is this still the case?

@MTRichards

This comment has been minimized.

Copy link

commented Sep 15, 2014

I personally like the possibility to reverse the sorting order as otherwise one has to scroll down the complete list. Currently I am thinking about a global switch, which is located on the bottom (or top) of the dialog called "reverse", which is a toggle button. What do you think?

I do like the reverse order - and your thought removes clutter.
@jancborchardt ?

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

@jancborchardt the issue with folder size was one of performance, if I remember. I don't know if we can get folder size directly from the server, or if we have to create it locally - if I remember, the issue was that we had to calculate it, which takes time with each sub folder query of the server - and gets worse the bigger the tree.

For test reasons I have added recursive function I have found on stackoverflow. It is only calculating the local folder size. But it seems very quick.
Or does it have to be the size of the folder on the server?

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 15, 2014

Let’s start small with just the one order and leave out the reverse, ok? Small steps please, we can always add it later if we feel it is necessary.

- Only three orders: A-Z, Big-Small, New-Old.
- Folder size is computed locally (therefore only downloaded files are
obeyed)
@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

Let’s start small with just the one order and leave out the reverse, ok? Small steps please, we can always add it later if we feel it is necessary.

Right. I have removed the other options.
Folder size is computed locally.

@davivel

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

Sorry for coming late.

About the size of folders, I think it is currently returned by the server in the property < d:quota-used-bytes > , but I am not sure, the name is a bit confusing. I would appreciate confirmation about this. CC @DeepDiver1975 @PVince81 .

@tobiasKaminsky , about your solution with local size of folders, probably won't work. The primary reason why we removed in the past the size of folders was not performance, but lack of synchrony with the real size in the server until the user browses down through all the tree of files or a full account synchronization is performed. With the local folder size the lack of synchrony is bigger, since will only match the server when all the files are downloaded (and synchronized).

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

@tobiasKaminsky by the way, you should join us on IRC in #owncloud-android (freenode IRC)! :)

@davivel

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

Yes, now I have a bouncer, so should not lost any more messages :$

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2014

About the size of folders, I think it is currently returned by the server in the property , but I am not sure, the name is a bit confusing. I would appreciate confirmation about this.
As far as I have found, the data are transferred via WebDav (ReadRemoteFolderOperation:79)
On the webdav interface via Browser (e.g. /remote.php/webdav/temp/) there is no file size.

Regarding the difference between local and server folder size:
I thought of this use case: Trying to find out which folder consume the most local size and then maybe delete them in order to get more space. But you are right, it is not mentioned that it is the local size which could confuse user.
So we should use the server folder size. But where is it? ;)

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2014

i have found something
in 049f659 @masensio has removed a function that gets the folder size from the server...
Maybe we can re-activate this and test the function?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

About the size of folders, I think it is currently returned by the server in the property < d:quota-used-bytes > , but I am not sure, the name is a bit confusing. I would appreciate confirmation about this. CC @DeepDiver1975 @PVince81 .

Folder size does not exist in the webdav standard

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

Folder size does not exist in the webdav standard

but there is an owncloud extension to the standard:
owncloud/core#10723

😉 🚀

@davivel

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

That's cool!!!

But I still do not understand what < d:quota-used-bytes > is. I cannot find it in Webdav standards, but seem to match the value of folders size.

@davivel

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

@tobiasKaminsky , that removed code is the one I mentioned before that does not work fine; it is calculating the folder size from size of descendent files -- but with the current known value of that sizes. If a file several levels down the way of the current file changed, that size won't be synchronized until the user browses down, and the size of its folder ancestor in the current folder will be calculated from a not synchronized value.

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

Yup, let’s move fast and iterate. No need to wait for folder size calculation, sorting by date and name is already great! :)

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2014

Ok.
Then I am done here :)
Please test and then it can be added to develop.

In order to get things clear it should exist a new issue for sorting by size, which refers to the "server-side folder size calculation" task. Otherwise it will maybe get forgotten?

@jancborchardt

This comment has been minimized.

Copy link
Member

commented Oct 13, 2014

@davivel @jabarros @rperezb please test ^

@davivel

This comment has been minimized.

Copy link
Member

commented Oct 14, 2014

I'm on it.

@@ -491,6 +491,40 @@ public boolean onOptionsItemSelected(MenuItem item) {
}
break;
}
case R.id.action_sort: {
SharedPreferences appPreferences = PreferenceManager
.getDefaultSharedPreferences(getApplicationContext());

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Is there any specific reason to pass getApplicationContext() instead of 'this' ? If the answer is 'no', 'this' should be prefered.

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

Changed.


// Read sorting order, default to sort by name ascending
Integer sortOrder = appPreferences
.getInt("sortOrder", 0);

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Please, define constants for the valid values of "sortOrder"; for instance, in FileListListAdapter.

For the "sortOrder" value itself a constant would be nice also, but I won't ask you for that because we didn't do our howework in the Preferences class :(

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

I wanted do use Enum for this, already had implemented it long ago, but unfortunately the SharedPreferences only allow Integer. So I am now using Integer as constants.

@@ -177,9 +197,13 @@ public View getView(int position, View convertView, ViewGroup parent) {
}
}
else {
if (FileStorageUtils.getDefaultSavePathFor(mAccount.name, file) != null){
fileSizeV.setVisibility(View.VISIBLE);
fileSizeV.setText(getFolderSizeHuman(FileStorageUtils.getDefaultSavePathFor(mAccount.name, file)));

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Sorting on size is disabled, but the (local) folder size is being shown. Should be commented too.

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

Done.


if(dir.exists()) {
long bytes = getFolderSize(dir);
if (bytes < 1024) return bytes + " B";

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Why adding this formatting? Would not be easier pass the result of getFolderSize(dir) to DisplayUtils.bytesHumanReadable(long bytes)?

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

Good point.
I was not aware of this function.

@@ -58,6 +66,9 @@
private FileDataStorageManager mStorageManager;
private Account mAccount;
private ComponentsGetter mTransferServiceGetter;
private Integer sortOrder;

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Please, keep the convention of naming member variables with an 'm' prefix.

sortOrder -> mSortOrder
sortAscending -> mSortAscending
appPreferences -> mAppPreferences

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

Done.

Collections.sort(mFiles, new Comparator<OCFile>() {
public int compare(OCFile o1, OCFile o2) {
if (o1.isFolder() && o2.isFolder()) {
return val * Long.compare(getFolderSize(new File(FileStorageUtils.getDefaultSavePathFor(mAccount.name, o1))),

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

Please, break the lines to be shorter than 120 characters; that's the maximum length shown by Github in pull requests without using the scrollbar.

* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, write to the Free Software

This comment has been minimized.

Copy link
@davivel

davivel Oct 17, 2014

Member

This file was entirely copied. I think it would be better placed in other part so that it's clear that it's being reused.

For the moment, move it to a new folder named something like src/third_parties/DaveKoeller/ ; place with it also a copy of the GPL license, version 2.1.

We have to think a bit how to place this & the external jars , and match it all with the update to Gradle build.

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 17, 2014

Author Contributor

Done.

@davivel

This comment has been minimized.

Copy link
Member

commented Oct 17, 2014

Sorry for the delay, I had to swtich to other tasks.

Finished the code review, please, check my comments.

Additionally, a rebase on develop is needed before testing, BUT not before we merge the release-1.6.0 branch into master & develop. We will do this on Monday.

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2014

I have committed all your requested changes.

&& file.getPermissions() != null && file.getPermissions().contains(PERMISSION_SHARED_WITH_ME));
}
private final static String PERMISSION_SHARED_WITH_ME = "S";

This comment has been minimized.

Copy link
@davivel

davivel Oct 20, 2014

Member

Did you enable auto formatting? We'd rather it's disabled until we unify an official formatter. This makes harder the reviews.

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 20, 2014

Author Contributor

Yes. It was a try to get automatically lines < 120 characters.
But it has changed to much, so I disabled it.

private Account mAccount;
private final ComponentsGetter mTransferServiceGetter;
private Integer mSortOrder;
public static final Integer mSortName = 0;

This comment has been minimized.

Copy link
@davivel

davivel Oct 20, 2014

Member

Please, use uppercase for constants.

mSortName -> SORT_NAME

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 20, 2014

Author Contributor

Done.


// Read sorting order, default to sort by name ascending
Integer sortOrder = appPreferences
.getInt("sortOrder", 0);

This comment has been minimized.

Copy link
@davivel

davivel Oct 20, 2014

Member

The default value 0 should be replaced by the appropiate constant for FileListListAdapter.

This comment has been minimized.

Copy link
@tobiasKaminsky

tobiasKaminsky Oct 20, 2014

Author Contributor

Done.

@davivel

This comment has been minimized.

Copy link
Member

commented Oct 21, 2014

Thanks for the updates. The code review is fine for me now.

Please, rebase on 'develop' branch so that @purigarcia may validate the functionality.

@davivel

This comment has been minimized.

Copy link
Member

commented Oct 24, 2014

We wanted to push this a bit up, so we rebased on 'develop' ourselves, in the branch 'sort_files'.

I close this PR to replace it with a #676

@davivel davivel closed this Oct 24, 2014

@masensio masensio referenced this pull request Mar 17, 2015

Closed

Add "sort by size" #895

1 of 1 task complete

@tobiasKaminsky tobiasKaminsky deleted the tobiasKaminsky:sortingFiles branch Aug 26, 2015

@tobiasKaminsky

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2016

@davivel I have created a PR to use the folder size of the server: #1655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.