Skip to content

Conversation

@AndyScherzinger
Copy link
Member

@AndyScherzinger AndyScherzinger commented Nov 1, 2019

Resolves #3263

TODO

  • if all folders have been hidden, empty view gets trigger (no way to toggle visibility on list anymore)
  • show/hide adapter flag to be fixed
  • display of visibility list toggle (footer) to be fixed
  • show action when all items have been hidden Show/Hide auto upload list items #4784 (comment)

Empty view in case all folders have been hidden before
device-2019-11-27-212308

List in case a folder is hidden (clicking the footer will show the hidden folders)
device-2019-11-02-185432

List with "show all" state
device-2019-11-02-175756

Menu of a (originally) hidden item in case you choses to show all items
device-2019-11-27-212330

Menu of a visible item
device-2019-11-27-212345

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Nov 2, 2019

@jancborchardt @nextcloud/designers I need some feedback/input on how to better hadnle certain things when it comes to show/hide items in the auto upload list. This PR adds this feature and does it as discussed in #4331 (comment) and #4331 (comment).

Summarizing the issues I am having (UI/UX wise at the moment)

  • there is no clear signed that the list is filtered/unfiltered (there is a footer in case you open the list and the list is filtered, else you don't have a footer --> not easy to discover)
  • you can't really toggle the filter-state of the list (you can only show hidden folders, then the footer magically disappears since there is nothing else to show on top, the only way to hide elements from the list is to navigate away and back to auto uploads...)
  • you can toogle show/hide on an item while the text is dynamic (action-based), so in case you have hidden items and hit the footer to show hidden items, the item is visible while the menu text says "show item" while this reflects the general visibility state it conflicts with the lists filter state.
  • In case all items have been hidden I show a special empty view with an action button to show the items (temporarily)

Looking at the description this all seems complex even to describe. I believe we can do better here 😃

So here are the screenshots 🖼 to give you an idea 💡 what I am talking 💬 about.

Empty view in case all folders have been hidden before (hidden folder info is kind of duplicated here...)
device-2019-11-02-175910

List in case a folder is hidden (clicking the footer will show the hidden folders)
device-2019-11-02-185432

List with "show all" state
device-2019-11-02-175756

Menu of a (originally) hidden item in case you choses to show all items
device-2019-11-02-175940

Menu of a visible item
device-2019-11-02-175824

Possible solutions

  • always show the footer and mirror the state (text to be discussed): "Show x hidden folders" / "Hide folders"
  • Add a checkbox to the menu item and use the text "hide item" (that way the state is clear (kind of fits how we display can read/edit/delete in the file details screen on Android and web
  • Also: If and how could hidden/non-hidden tems be distinguished within the list? (in case we want that)

@nextcloud nextcloud deleted a comment Nov 2, 2019
@codecov
Copy link

codecov bot commented Nov 2, 2019

Codecov Report

Merging #4784 into master will increase coverage by 0.15%.
The diff coverage is 16.57%.

@@             Coverage Diff              @@
##             master    #4784      +/-   ##
============================================
+ Coverage     17.58%   17.74%   +0.15%     
  Complexity        3        3              
============================================
  Files           384      384              
  Lines         32528    32650     +122     
  Branches       4588     4597       +9     
============================================
+ Hits           5720     5793      +73     
- Misses        25887    25931      +44     
- Partials        921      926       +5
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/owncloud/android/db/ProviderMeta.java 84.61% <ø> (ø) 0 <0> (ø) ⬇️
...oud/android/datamodel/SyncedFolderDisplayItem.java 50% <ø> (ø) 0 <0> (ø) ⬇️
...ncloud/android/datamodel/SyncedFolderProvider.java 11.66% <0%> (-0.2%) 0 <0> (ø)
.../dialog/SyncedFolderPreferencesDialogFragment.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...droid/ui/dialog/parcel/SyncedFolderParcelable.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...n/java/com/owncloud/android/jobs/FilesSyncJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...wncloud/android/providers/FileContentProvider.java 21.6% <0%> (-0.24%) 0 <0> (ø)
...ncloud/android/ui/adapter/SyncedFolderAdapter.java 13.14% <13.97%> (-1%) 0 <0> (ø)
...a/com/owncloud/android/datamodel/SyncedFolder.java 51.51% <28.57%> (+1.51%) 0 <0> (ø) ⬇️
...oud/android/ui/activity/SyncedFoldersActivity.java 26.81% <30.4%> (+5.01%) 0 <0> (ø) ⬇️
... and 9 more

@nextcloud nextcloud deleted a comment Nov 2, 2019
@nextcloud nextcloud deleted a comment Nov 2, 2019
@nextcloud nextcloud deleted a comment Nov 2, 2019
@nextcloud nextcloud deleted a comment Nov 2, 2019
@nextcloud nextcloud deleted a comment Nov 3, 2019
@AndyScherzinger AndyScherzinger added needs info Waiting for info from user(s). Issues with this label will auto-stale. design labels Nov 3, 2019
@jancborchardt
Copy link
Member

Alright! Only have 2 pieces of feedback:

The emptycontent view could do without a button, instead the subline just saying "Show 2 hidden folders", then it’s consistent, doesn’t duplicate, and doesn’t call to attention what someone hid in the first place.

Add a checkbox to the menu item and use the text "hide item" (that way the state is clear (kind of fits how we display can read/edit/delete in the file details screen on Android and web

Yes please, that makes it more obvious. I was a bit confused when I saw the screenshot with the menu open saying "Show folder" cause it was already shown.

@AndyScherzinger
Copy link
Member Author

Thanks for the feedback @jancborchardt ❤️

@nextcloud nextcloud deleted a comment Nov 5, 2019
@nextcloud nextcloud deleted a comment Nov 5, 2019
@AndyScherzinger AndyScherzinger added feature: auto upload and removed needs info Waiting for info from user(s). Issues with this label will auto-stale. labels Nov 5, 2019
AndyScherzinger and others added 7 commits November 27, 2019 21:34
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
… + action button

Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…ded boolean boxing

Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
… [skip ci]

Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…display

(design review)

Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
@AndyScherzinger AndyScherzinger force-pushed the hideAutoUploadEntries branch 2 times, most recently from 5767c30 to 553df43 Compare November 27, 2019 20:39
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
@AndyScherzinger
Copy link
Member Author

@tobiasKaminsky @ezaquarii all your review comments have been fixed, thanks for the review. Would you mind giving it another look / test drive? 🙏

@ArisuOngaku Since you made some changes to these files recently and I had to resolve some larger conflicts due to the fact that I moved some code around I'd appreciate if you could also have a look so I don't break your recent changes (which I shouldn't have, but one never knows...) ❤️

… [skip ci]

Signed-off-by: nextcloud-android-bot <android@nextcloud.com>
@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11769.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
- Added 1
           

Complexity increasing per file
==============================
- src/main/java/com/owncloud/android/ui/adapter/SyncedFolderAdapter.java  2
- src/main/java/com/owncloud/android/providers/FileContentProvider.java  3
- src/main/java/com/owncloud/android/ui/dialog/parcel/SyncedFolderParcelable.java  2
         

See the complete overview on Codacy

@Override
public void onCancelSyncedFolderPreference() {
mSyncedFolderPreferencesDialogFragment = null;
syncedFolderPreferencesDialogFragment = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nextcloud-android-bot
Copy link
Collaborator

Codacy

333

Lint

TypemasterPR
Warnings5959
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings104
Security Warnings44
Dodgy code Warnings136
Total407

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings117
Security Warnings44
Dodgy code Warnings134
Total418

Copy link
Contributor

@ashpieboop ashpieboop left a comment

Choose a reason for hiding this comment

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

@AndyScherzinger everything looks fine regarding my PR. You only modified lines (files) in a way that can't undo my fix. I see no regression! :)

@AndyScherzinger
Copy link
Member Author

Thanks a lot @ArisuOngaku ❤️ Highly appreciate you taking a look 🎉

@tobiasKaminsky
Copy link
Member

Nice, let us merge this :-)

But I have some things, which we can do in new PRs, if approved:

  • change "show x folders" in bottom to "temporary show x 4 folders" as they are gone when re-opening the auto upload view
  • overlay menu looks a bit big (in height)?
    2019-11-28-081301
  • position of checkbox looks a bit off
    2019-11-28-081437

@tobiasKaminsky tobiasKaminsky merged commit 2c57e37 into master Nov 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the hideAutoUploadEntries branch November 28, 2019 07:19
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.10.0 milestone Nov 28, 2019
tobiasKaminsky added a commit that referenced this pull request Nov 29, 2019
2c57e37 Merge pull request #4784 from nextcloud/hideAutoUploadEntries
5257fb2 [tx-robot] updated from transifex
aa43181 Merge pull request #4882 from nextcloud/dependabot/gradle/com.google.android-flexbox-2.0.0
1f4e857 Drone: update FindBugs results to reflect reduced error/warning count [skip ci]
d17c853 re-add button/action and improve upon code review
74bfe47 remove action button, use checkable menu item for shown/hidden state display
3e872df add more spotbug exclusions (for 3rd party libs)
4e0b405 Drone: update FindBugs results to reflect reduced error/warning count [skip ci]
7c62ad6 unify use of boolean, make use of lombok getter/setters, remove unneeded boolean boxing
bedc2b4 housekeeping, removing unused resources
ca6918f show/hide improvements when list is completed hidden + nicer empty UI + action button
23e4424 show/hide auto upload list items
540c649 Merge pull request #4894 from nextcloud/dependabot/gradle/kotlin_version-1.3.61
f34ba2c Bump kotlin_version from 1.3.60 to 1.3.61
48038d5 Merge pull request #4883 from nextcloud/dependabot/gradle/io.gitlab.arturbosch.detekt-detekt-gradle-plugin-1.2.0
83e0ebc [tx-robot] updated from transifex
21c0de3 daily dev 20191127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The ability for users to more manually edit/delete/hide their auto-upload media folder connections.

7 participants