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

item data (size, last modified) in the file's list cell detail label #117

Merged
merged 5 commits into from
Sep 27, 2018

Conversation

pablocarmu
Copy link
Contributor

Description

Instead of the mime-type this PR write in the detail label the data in a readable way i.e. 54 Mb and the last modified date i.e. 06 sept, 2018

Related Issue

#62

Motivation and Context

This provides more information to the user and makes sense because we can sort by date and size

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

…m+Extension.

- Now in the ClientItemCell's detail label appears the size and the last modified date for every item.
@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #117 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #117     +/-   ##
=========================================
- Coverage   27.17%   27.07%   -0.1%     
=========================================
  Files          63       63             
  Lines        5741     5762     +21     
=========================================
  Hits         1560     1560             
- Misses       4181     4202     +21
Impacted Files Coverage Δ
ownCloud/Client/ClientItemCell.swift 0% <0%> (ø) ⬆️
ownCloud/SDK Extensions/OCItem+Extension.swift 3.7% <0%> (-0.5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d9ae6f...ec69c58. Read the comment docs.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 26, 2018

Regarding #62, in case of folders is included the number of items instead of the folder size. I personally prefer the folder size.

What do you think? @javiergonzper @mneuwert @michaelstingl @felix-schwarz

@michaelstingl
Copy link
Contributor

@pablocarmu @jesmrec screenshots?

@jesmrec
Copy link
Contributor

jesmrec commented Sep 26, 2018

screen shot 2018-09-26 at 15 39 28

@michaelstingl
Copy link
Contributor

Display both?

Finder on macOS;

107.469.460.299 bytes (746,3 MB on disk) for 1.680 items

That's too much detail, but only size and number of items?

@javiergonzper
Copy link
Contributor

javiergonzper commented Sep 26, 2018

Regarding #62, in case of folders is included the number of items instead of the folder size. I personally prefer the folder size.

What do you think? @javiergonzper @mneuwert @michaelstingl @felix-schwarz

On Files App they show the size only on the files. Not on the folders. But probably it is because that information could no be available (calculate the size of a folder with subfolders is not an easy task and maybe not all the cloud services return it).

To be consisten I also prefer the size of the folder but not sure what it is more useful for a normal user.

Files app:

image1 1

@javiergonzper
Copy link
Contributor

Display both?

Finder on macOS;

107.469.460.299 bytes (746,3 MB on disk) for 1.680 items

That's too much detail, but only size and number of items?

Size, number of items and last modification date. Too much in my opinion.

@michaelstingl
Copy link
Contributor

Okay, then size is more useful I think…

Copy link
Contributor

@mneuwert mneuwert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesmrec is folder size expensive to calculate or is it something you get for free via server API?

@jesmrec
Copy link
Contributor

jesmrec commented Sep 26, 2018

@mneuwert we take the size from API

@michaelstingl
Copy link
Contributor

ownCloud server might not know the size from folders that are mounted storages. That needs to be covered as well…

@pablocarmu
Copy link
Contributor Author

pablocarmu commented Sep 26, 2018

From the app's perspective, I'm showing in the detail the size and lastModifiedDate properties of OCItem with a nice format.

Regarding #62, in case of folders is included the number of items instead of the folder size. I personally prefer the folder size.

There is no current property in OCItem to know the number of child items of a folder and I think that unless you make a PROPFIND to get the contents of this folder and count the results you can not get it.

ownCloud server might not know the size from folders that are mounted storages. That needs to be covered as well…

Right now if there is no info about the size we're showing zero as displayed here. I wanted to mimic the behavior of the old app here, but instead of showing minute details I coded the date by days ie. Today, Tomorrow, 3 days ago,...

@jesmrec
Copy link
Contributor

jesmrec commented Sep 26, 2018

i will check what happens after mounting an external storage... but the app will show what the server retrieves.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 27, 2018

In empty folders you set Zero KB

I'd set 0 KB , 0 B (quota notation), or 0 bytes.

- Removed countStyle because .file is the default option.
@pablocarmu
Copy link
Contributor Author

I changed the property allowsNonnumericFormatting to false to not allow non-numeric display options. Now it will show 0 bytes

@jesmrec
Copy link
Contributor

jesmrec commented Sep 27, 2018

Regarding external storage, we can not know the folder and file sizes till we browse through it. PROPFIND returns:

<oc:size>-1</oc:size>

with such value, the web UI sets Pending as folder size, and set a different icon. We can distinguish such kind of folders with the permissions property, including M.

Icon is not in the scope of the current PR.

@jesmrec
Copy link
Contributor

jesmrec commented Sep 27, 2018

External storage issue fixed. From my side, this is OK to move forward... after code review (@mneuwert )

@jesmrec jesmrec added the Approved by QA Approved by QA label Sep 27, 2018
@jesmrec jesmrec merged commit 33e317e into master Sep 27, 2018
@jesmrec jesmrec deleted the feature/itemDataInFileList branch September 28, 2018 06:47
@jesmrec jesmrec mentioned this pull request Jan 22, 2019
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved by QA Approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants