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

Add "Recent" file listing #292

Merged
merged 8 commits into from
Jul 25, 2016
Merged

Add "Recent" file listing #292

merged 8 commits into from
Jul 25, 2016

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jul 2, 2016

Show a list of recently modified files and folders

recent

On showing folders

How to show folders in the list is the most complex part of this PR, since the mtime (both on disk and in cache) of a folder gets changed when a file in the folder is modified simply just showing folders in the same way as files would result in the parent folder being shown together with the most recently changed file in that folder.

Ideally we would show folders based on their creation time but unfortunately we don't save that information, instead this PR uses the modified date of the least recently changed file in the folder which isn't perfect (especially for folders containing a single file) but it works decently.

I could also be better to hide most folders altogether in most cases

On hiding folder contents

Another thing I was thinking about to make the recent list more useful is hiding the contents of newly uploaded folders, currently if you upload a large folder the contents of it will take over the entire recents list.
Instead it would be nice if the recents list would only show the newly uploaded folder, with the newly uploaded contents being implicitly grouped in the folder.

Further work (maybe for nc+1)

  • unit tests
  • make the list as useful as possible
    • maybe hide folders where they don't add value
    • hide content of folders where they don't add value
  • Icon for the files sidebar
  • Maybe allow specifying the range mtimes to be searched for (show me files modified 2 days ago)

cc @jancborchardt @LukasReschke

@icewind1991 icewind1991 added the 2. developing Work in progress label Jul 2, 2016
@icewind1991 icewind1991 added this to the Nextcloud Next milestone Jul 2, 2016
@MariusBluem
Copy link
Member

Wow... Amazing 👊
How is Google Drive, OneDrive or Dropbox handling folder contents?

@Bugsbane
Copy link
Member

Bugsbane commented Jul 3, 2016

Love, love, love this! I can create an icon if needed. My first thought though, is that a "reload" type icon would work just fine here though, as would a clock. Gnome 3's nautilus kind of combines both which I quite like, while KDE's Dolphin uses a calendar, which I'm not fond of, as generally people are looking for files modified in the last few minutes/hours, not days/weeks.

What think the rest of @nextcloud/designers? My preference is something Nautilus-like (despite being a KDE fanboy) such as this.

@MorrisJobke
Copy link
Member

What think the rest of @nextcloud/designers? My preference is something Nautilus-like (despite being a KDE fanboy) such as this.

cc @nextcloud/designers

*
* @return DataResponse
*/
public function getRecentFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

Unit test for this? 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

In the todo 😄

@williambargent
Copy link
Member

👍 Looks good, Just need the Icon. @Bugsbane I like the idea of some sort of clock face. Would be awesome if we could get it to actually show the time of the last file which was modified (Would probably cause to many issues).
time-512

@icewind1991
Copy link
Member Author

How is Google Drive, OneDrive or Dropbox handling folder contents?

It gets all it's data from the cache, as long as changes are scanned they should show up

@jancborchardt
Copy link
Member

Ok, so how does this exactly compare to »Sort by modified date«?

I think the general thing is cool and all, I am just worried we're adding more and more stupid sections to the navigation without properly connecting everything. That is, I think a much better solution would be to:

  • use »Sort by modified date« as default in the all files list
  • of course remember if that sorting is changed, on a per-user basis
  • (just to bring an example of another nav section we can integrate better) we should always sort the favorite files of that folder up top

@icewind1991
Copy link
Member Author

Ok, so how does this exactly compare to »Sort by modified date«?

The recents "tab" shows recent files across all folders, not just the current folder

@Bugsbane
Copy link
Member

Bugsbane commented Jul 4, 2016

This is exactly the same as how file browsers let you both sorry by date, but also see all of the files you've modified recently, regardless of location. I use both daily, for different purposes.

@jancborchardt
Copy link
Member

It was partly a rhetorical question. ;) What I meant to say is that my proposal above would already take care of a majority of cases for the »Recent« section, but in a less interface-filling and more integrated way.

Let's take small steps, and find how we can do more with less. Again sure, we can just add stuff, but that's the trap software falls into before people abandon it cause it's too complicated.

@icewind1991 icewind1991 added 3. to review Waiting for reviews 2. developing Work in progress and removed 2. developing Work in progress 3. to review Waiting for reviews labels Jul 6, 2016
@Bugsbane
Copy link
Member

Bugsbane commented Jul 7, 2016

draft mockup of a clock style icon for recently modified files:
recent-icon-1

@jancborchardt
Copy link
Member

@Bugsbane very nice! :) I'd only increase the size by 1px for each side because compared to the square icons around it looks a tad small. That's because it has the same width as the folder or share icons, but being a circle it needs a bit more size to appear visually sized correctly.

@Bugsbane
Copy link
Member

Bugsbane commented Jul 7, 2016

Ok, this is as large as it can go, while still fitting into the 16x16 png image size:
recent-icon-1

If it's ok, just let me know and I'll svgo/trimage it and commit (what path / filename should I commit the svg/png to? I don't see any image name in Icewind's commit...)

@icewind1991
Copy link
Member Author

icewind1991 commented Jul 7, 2016

png

master will no longer have fallback png's so no need to add a png version

what path / filename should I commit the svg/png to?

You'll need to add the nav-icon-recent class (see apps/files/css/files.css line ~90) which points to the newly added icon

@Bugsbane
Copy link
Member

Bugsbane commented Jul 7, 2016

Ah, ok. It's being pulled in by css. Thanks. I just did a quick search on the commit changes page for .png and .svg and didn't see anything, and assumed you hadn't added the icon yet as it didn't exist. I'll add the class.

@jancborchardt
Copy link
Member

@Bugsbane looks great! Please make sure the circle is pixel-perfect, that is it should be either 1px or most probably 2px width. That will ensure it will look sharp and not blur.

@Bugsbane
Copy link
Member

Bugsbane commented Jul 7, 2016

Ok, I've submitted PR #340 and made sure that as much as is possible, that everything uses whole pixel stroke widths and aligns with the pixel grid. One thing puzzles me though. I could see the class pulling in the svg, but I didn't see anything referring to the PNG fallbacks, although I figure that these substitutions are probably done automatically by JS rather than being hard coded.

@Bugsbane
Copy link
Member

Bugsbane commented Jul 7, 2016

Oh, and I tried svgo on it, but no matter how I tried to modify the original SVG, svgo always butchered the image, completely removing the hands of the clock or worse. Accordingly I ran sourge over it instead, which worked fine.

@LukasReschke LukasReschke force-pushed the recent-files branch 2 times, most recently from 3ee1621 to d425040 Compare July 8, 2016 10:29
Bugsbane pushed a commit to Bugsbane/server that referenced this pull request Jul 9, 2016
Added the class "nav-icon-recent" to display the new "recent files" file list view to be added by @icewind1991 in PR nextcloud#292

It's now placed after nav-icon-files which was where @jancborchardt was suggesting iirc.
Bugsbane pushed a commit to Bugsbane/server that referenced this pull request Jul 9, 2016
Add the new icon for "Recent files" file list view in the files app by @icewind1991, as discussed in PR nextcloud#292

Icon is already compressed/cleaned by scour rather than svgo as svgo seems to corrupt it.
@icewind1991
Copy link
Member Author

  • Fixed texteditor
  • Disallow sorting
  • Fixed no files showing up in some cases

This is good for re-review ( @jancborchardt @schiessle @MorrisJobke ) any open tasks in the original post are things we can look into for a further release

@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 22, 2016
@MorrisJobke
Copy link
Member

I tested it and it works 👍

@MorrisJobke
Copy link
Member

There was 1 failure:

1) OCA\Files\Tests\Controller\ViewControllerTest::testIndexWithRegularBrowser
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
                         'appname' => 'files'
-                        'script' => 'list.php'
+                        'script' => 'recentlist.php'
                         'order' => 2
                         'name' => 'Recent'
                         'active' => false
                         'icon' => ''
                     )
                     2 => Array (...)
                     3 => Array (...)
                     4 => Array (...)
                     5 => Array (...)
                     6 => Array (...)
                     7 => Array (...)
                 )
             )
             'l10n' => OC\L10N\L10N Object (...)
             'theme' => OC_Defaults Object (...)
         )
         'appContents' => Array (...)
     )
     'renderAs' => 'user'
     'appName' => 'files'
     'headers' => Array (...)
     'cookies' => Array ()
     'status' => 200
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\ContentSecurityPolicy Object (...)
 )

/drone/src/github.com/nextcloud/server/apps/files/tests/Controller/ViewControllerTest.php:318

@williambargent
Copy link
Member

👍 tested and works, it is missing the favourites column, I think this should still be included in the view. Without it looks a bit odd transitioning from Files to Recent as everything shifts over to the left.

@MorrisJobke
Copy link
Member

Without it looks a bit odd transitioning from Files to Recent as everything shifts over to the left.

The other views also don't have the favorite column. I would leave it like it is now.

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 25, 2016
@williambargent williambargent merged commit 352e24e into master Jul 25, 2016
@williambargent williambargent deleted the recent-files branch July 25, 2016 14:25
@williambargent
Copy link
Member

For consistency, it might be worth reviewing and adding favourites to all the views.

@MariusBluem
Copy link
Member

Is this something we want to have for Nextcloud 10? @karlitschek

@MorrisJobke
Copy link
Member

Is this something we want to have for Nextcloud 10? @karlitschek

I would say no, because this is clearly a feature and we only back port bug fixes.

@karlitschek
Copy link
Member

Nice. I agree that we shouldn't backport this for now

@jancborchardt
Copy link
Member

@williambargent can you open a new issue about adding the favorites column to all views? It should indeed be there for everything, including search results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants