Skip to content

Conversation

@AndyScherzinger
Copy link
Member

rebased and adapted PR originally done by @tobiasKaminsky to switch between all and offline files via the drawer menu.

please review @przybylski @tobiasKaminsky

cc @LukasReschke and @jancborchardt for feature review in general even though this is an oldie PR :)


private void fetchFavoritesToSyncFromLocalData() {
List<OCFile> children = mStorageManager.getFolderContent(mLocalFolder);
List<OCFile> children = mStorageManager.getFolderContent(mLocalFolder, true);
Copy link
Member Author

Choose a reason for hiding this comment

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

@tobiasKaminsky wasn't sure what to put in here, decided to use true, but might also be false. You might know/recall :)

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand all files within the folder (childrens) should be fetched, therefore changing it to "false".

@AndyScherzinger
Copy link
Member Author

Mergable from my point of view

LGTM

@AndyScherzinger
Copy link
Member Author

@przybylski @tobiasKaminsky what do you think? I tested it on my device and it works (for me)

// MainApp.showOnlyFilesOnDevice(true);
// refreshDirectory();
// break;
case R.id.nav_on_device:
Copy link
Member

Choose a reason for hiding this comment

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

show on device and show all items are mutually exclusive, so cannot be checked simultaneously. And yet, from code point of view it looks like they can. But I can be mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @przybylski they are mutually exclusive. Can you go a bit more into details why they could be checked simultaneously? The menu in general only allows one item to be in the checked state: https://github.com/nextcloud/android/blob/master/res/menu/drawer_menu.xml#L24

Copy link
Member

Choose a reason for hiding this comment

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

This convinces me :) I haven't checked the xml file. Sorry

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I had to check the menu myself too 👍

@przybylski
Copy link
Member

👍

@AndyScherzinger
Copy link
Member Author

@tobiasKaminsky can you have a final look for the LGTM thingy 😉

@jancborchardt
Copy link
Member

Nice, good stuff! :) We should push forward more with the offline stuff. The limited offline capability of the app was always a shortcoming – especially when you are on a train, plane or with bad mobile connection.

@AndyScherzinger AndyScherzinger added this to the Nextcloud App 1.2.0 milestone Jul 14, 2016
@AndyScherzinger
Copy link
Member Author

@jancborchardt that part is still the same (at the moment), this PR basically filters out all non-offline-available files, so you only see files already on your phone so you don't accidentally download a file.

@jancborchardt
Copy link
Member

jancborchardt commented Jul 14, 2016

Yup yup, I know – the sidebar entry. :) Good stuff 👍

@AndyScherzinger
Copy link
Member Author

@tobiasKaminsky can you check to code and if fine with you, merge? 🚀

@jancborchardt
Copy link
Member

(Btw, let’s keep the thumbs up emoji as an indicator for review. We use that since ages now. ;) And l g t m is not really understandable to many people cause it’s a really bad abbreviation.)

@AndyScherzinger
Copy link
Member Author

OK! :) but for the plugin you can go for LGTM, 👍 or :shipit: to approve (plugin enforced to MAINTAINERS responding with one of these) in order to be able to merge... 💃

@jancborchardt
Copy link
Member

jancborchardt commented Jul 14, 2016

Ah k, was just confused cause someone changed/added the +1 in my comment to LGTM (cause I didn’t write that)

@AndyScherzinger
Copy link
Member Author

Yeah, that was me... 😁

@tobiasKaminsky
Copy link
Member

LGTM

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.

4 participants