android icon indicating copy to clipboard operation
android copied to clipboard

MATERIAL DESIGN: As an OC user I want that the new list & grid components from Material Design replace the current so that performance is improved in folders with hundreds of files & images

Open davivel opened this issue 10 years ago • 42 comments

Google states that the new elements for lists and grid view included in the Android Support Library help to improve the performance of lists and grids with tons of elements. I couldn't check it, but it's obvious they enforce the recycling of views instead of simply allowing the option to do it (or the option to forget to do it and have problems; see #955 fixing it). If there is also a better recycling policy behind, we will only notice it by start using them.

Nevertheless, there are other important reasons to embrace this change:

  • The GridView widget currently in use is a mess; it's not fully implemented in all the Android versions (even in different 4,x versions). This fact already caused crashes in the past, and is still enforcing some undesired graphical effect in older 4.x devices (my lips are sealed). Using the new Grid element in Android Support Library we will have a single and complete implementation for all our grids.
  • Both components are needed if we want to take advantage of new elements for cool animations in Material Design.

Sure of interest for @tobiasKaminsky , @jancborchardt , and everybody is welcome.

Just please, remember, that #692 should be done first (or at least the part of the build target).


Details

Branch

Implementation will be done on feature branch https://github.com/owncloud/android/tree/984_recycler_view

Issues

Issues referenced/mentioned in this issue and their current status:

  • [x] #955 - MERGED - HOTFIX: prevent OOM when scrolling Thumbnails
  • [x] #692 - MERGED - Update to Material design: add OC THEME based on Material Design THEME
  • [x] #1090 - MERGED - Material buttons and checkboxes
  • [x] #1076 - MERGED - Material Design
  • [x] #1100 - MERGED - Material FAB with speed dial implementation
  • [x] #1473 - MERGED - Move to compile target Marshmallow (Android 6 - v23) - rebased without FAB
  • [x] #1559 - TO-REVIEW - Toolbar, new Drawer and new Account Manager

TODOs

  • [x] Bump support lib to latest version - #1557
  • [ ] Add recycler view dependency for gradle and ant
  • [ ] implement recycler view for list and grid

davivel avatar May 21 '15 11:05 davivel

Grid lists and lists from Material Design http://www.google.com/design/spec/components/grid-lists.html# http://www.google.com/design/spec/components/lists.html

masensio avatar Jun 03 '15 11:06 masensio

why just not use recycler view and LayoutManager to show list or Grid? Plus its ViewHolder implementation would save a lot of resources, because currently what we have implemented as ViewHolder in FileListListAdapter is laughable.

ddijak avatar Aug 08 '15 16:08 ddijak

@skymania actualy, that's the whole point of this issue.

Kernald avatar Aug 08 '15 16:08 Kernald

I hope so.

ddijak avatar Aug 08 '15 16:08 ddijak

@skymania since it sounds like you’re experienced, maybe you’re interested in contributing? :) I’m sure if you need any help, @davivel @tobiasKaminsky @AndyScherzinger can help you.

jancborchardt avatar Aug 08 '15 22:08 jancborchardt

Absolutely! :) The material_toolbar branch is probably a good starting point since it contains most of the material implementation and the latest AppCompat release.

AndyScherzinger avatar Aug 08 '15 23:08 AndyScherzinger

I have implemented toolbar, recycler view and UIL. Currently Im working on navigation drawer but time is not currently on my side so I have no idea when I will have the time to proceed with material implementations. I have to upload all the changes I made on github tho.

ddijak avatar Aug 09 '15 21:08 ddijak

@skymania please do a pull request even when it's work in progress so we can already review. If a pull request is too big, risk is high it's very difficult to merge.

jancborchardt avatar Aug 09 '15 23:08 jancborchardt

@ all how should we all move forward with this overall topic, since we now have this, #1076 and #1090 and @skymania provides the next implementation (which is fine with me) and I provided the action bar variant. So imho we need some kind of coordination to not implement everything twice :)

AndyScherzinger avatar Aug 10 '15 08:08 AndyScherzinger

I’d say we should wait for #1076 to be QA'd and merged – which will hopefully happen soon. Then #1090 shortly after (we need it in the next cycle @davivel).

Then after that we can build on top of that. Otherwise it just becomes unmanageable. There’s no reason to cram all of these improvements in such a short amount of time now. ;)

jancborchardt avatar Aug 10 '15 14:08 jancborchardt

Sounds reasonable @jancborchardt - thanks for the clarification :)

AndyScherzinger avatar Aug 10 '15 15:08 AndyScherzinger

Finally found some time and I am currently using this for my owncloud server. Still in development tho. Made huge code optimizations so everything is running smooth as it should be.

ddijak avatar Nov 26 '15 13:11 ddijak

@skymania did you rewrite one of the material branches of the ownCloud Android app?! :question: @jancborchardt I really do like the way how @skymania is visualizing the link/download with the round icon :+1: more colors/contrast and a bold style (material style)

AndyScherzinger avatar Nov 26 '15 15:11 AndyScherzinger

the master one. 1.8.0

ddijak avatar Nov 26 '15 16:11 ddijak

hmm, okay. The fab implementation would have been on the material_fab branch already ;)

AndyScherzinger avatar Nov 26 '15 16:11 AndyScherzinger

Any news on this matter? @skymania fancy some collaboration?

I would start working on this and will create a new branch for the recycler view implementation based on the actual master a.k.a soon to be v2.0.0 all formerly mentioned PRs and issues have been merged or are at least done from a implementation point of view only missing out on review/test/merge.

I will update the issue referencing all the issues. - DONE

Branch has been created: https://github.com/owncloud/android/tree/984_recycler_view So who every wants to work with me on this, feel free to do so either via direct commits to the branch or PRs targeting 984_recycler_view

AndyScherzinger avatar Apr 19 '16 07:04 AndyScherzinger

also looping in @malkomich since this is an issue raised in 2015 :)

AndyScherzinger avatar Apr 19 '16 08:04 AndyScherzinger

@AndyScherzinger what ever you need you can take from Cirrus. It already has a recycler view implementation.

ddijak avatar Apr 19 '16 11:04 ddijak

That would be awesome! :+1:

Two though, just to be save:

  • Could you sign the contributor agreement https://owncloud.org/contribute/agreement/ ? (If I re-use your implementation and not do it from scratch on my own the code couldn't be merged)
  • Would you fancy/do code reviews on my changes / your"copied" code?

AndyScherzinger avatar Apr 19 '16 13:04 AndyScherzinger

@skymania , any chance you make a PR from your fork to ownCloud with the changes for this topic?

davivel avatar Apr 19 '16 16:04 davivel

@AndyScherzinger , I disabled checkbox on #1559 since it's not merged yet; was a bit confusing. Hope it's OK to you.

davivel avatar Apr 19 '16 16:04 davivel

Sure, you mean 1559 (?) I guess (drawer) which has just been mentioned because the discussion in this issue simply mentioned drawer, so I referenced it to explicitly document that that part has already been done.

AndyScherzinger avatar Apr 19 '16 16:04 AndyScherzinger

That was, I fixed the comment.

My point is, over here we reserve the word "done" for things tested and merged. In this case I had said "developed". I know, too punctilious for this time... :D

davivel avatar Apr 19 '16 16:04 davivel

@AndyScherzinger so I sign contribution agreement and we can start? I would love to contribute and do code reviews, but just to warn you, we will have to do shit loads of code changes to get this right :D

ddijak avatar Apr 25 '16 08:04 ddijak

That's nice, @skymania .

If you had to do a summary, what would those lot of changes include?

davivel avatar Apr 25 '16 08:04 davivel

@davivel

  • implementation of recycler view
  • search
  • totally changed image loading mechanism (the current one is blocking UI)
  • avoiding multiple calls to notifyDataSetChanged (this is happening now for loading content at start and then for every call we get back from broadcast recevier and its blocking UI in folders with lot of contents, for example folders with lots of images)
  • async fill and notify adapter that the data has changed

and probably something else that I have forgot now :)

ddijak avatar Apr 25 '16 08:04 ddijak

@skymania yes that is basically it :) After signing the agreement and sending it to @karlitschek you will get the corresponding access rights to the repo and we can start working on a branch in the owncloud repo itself :)

I am fully aware that it is a huge amount of work that needs to be done, probably larger than what you had to do for cirrus since we do and (and will have) at least two more requirements for the lists:

  • multi select support for @tobiasKaminsky's change
  • header(s) and footer e.g. for the new uploads view introduced in v.2.0.0
  • (switch) between list/grid/grid-image item views (kind of like your implementation)

I am totally in line with your list that will need to be implemented. I also think we will need an abstract implementation for reuse throughout the lists that is able to do the things you and I mentioned.

Really looking forward to the collaboration and finally bringing RecyclerView to the app :+1:

Besides that, #1559 also changes quite a lot just to introduce a new drawer. :)

AndyScherzinger avatar Apr 25 '16 08:04 AndyScherzinger

The agreement can be found here: https://owncloud.org/contribute/agreement/ When I signed it I just sent a scan, which was totally fine :)

AndyScherzinger avatar Apr 25 '16 08:04 AndyScherzinger

@AndyScherzinger done. Lets do some damage :)

ddijak avatar Apr 25 '16 08:04 ddijak

Sounds great, guys.

But please, don't forget that we need to keep an order so that the process is manageable. All those changes shouldn't go in a single branch. Each of them should go in a different branch created from master, and target master when done in separate PRs.

I'm eager to see how this progresses :)

davivel avatar Apr 25 '16 08:04 davivel