Skip to content

Conversation

@mario
Copy link
Contributor

@mario mario commented Sep 12, 2017

Signed-off-by: Mario Danic mario@lovelyhq.com

Favorites, Photos and Shared files somehow stopped working. This fixes it.

Please test for possible issues with search overall, rotation etc and fix if there's something wrong.

resolves #1535

@AndyScherzinger
Copy link
Member

See attached some screenshots... unfortunately I won't have the time to fix it since I am not an expert on this part of the code base so it would also take me much more time to fix.

device-2017-09-13-190034
device-2017-09-13-190112
device-2017-09-13-185958

  • Rotation in general works fine, except for search via the tool bar, see screenshots the search doesn't stretch all over the bar (might be okay for 2.0.0)
  • favs/shared works on my phone also no issues with rotation
  • what doesn't work and maybe never did (yet) is back navigation:
    • searching and then hitting the back arrow or "X" will close the search bar but leave you with the search result and the drawer icon
    • entering a folder (found by search) and hitting the back arrow navigates you to the root folder with search result/toolbar state reset.

@tobiasKaminsky @mario how should we proceed here? From what I understood from @mario's comment search hasn't worked at all so this would be an enhancement while I can't say since I haven't used/tested it before for quite a while...

@mario
Copy link
Contributor Author

mario commented Sep 14, 2017

@AndyScherzinger I don't know shrug. Back when search/favorites/photos/etc was written, tested and approve it worked and it streched the majority of action bar. Dunno what happened but my PR at least fixes part of things.

Hopefully, @tobiasKaminsky has the time since he did the changes to the initial search implementation.

@tobiasKaminsky
Copy link
Member

what doesn't work and maybe never did (yet) is back navigation:

  • searching and then hitting the back arrow or "X" will close the search bar but leave you with the search result and the drawer icon
  • entering a folder (found by search) and hitting the back arrow navigates you to the root folder with search result/toolbar state reset.

This was already discussed in
#686
with a potential PR in #1272, but:
#1272 (comment)

So I've made some progress on this. However, it requires some changes to the overall structure of the app in the fact that all these previews should not inherit from FileDisplayActivity, but should instead have their own activity. Once this is done, the search will be trivial.
Postponing this to currently 1.5.1 (or 2.0.1, whatever).

So I would be fine with a "basic" search in 2.0 with the above and known disadvantages.
And for 2.0.1 or 2.1 we need to enhance this.

If you are fine with this, I will test the PR only in the scope of working search/favs and then we can merge this.

@tobiasKaminsky tobiasKaminsky mentioned this pull request Sep 21, 2017
57 tasks
@AndyScherzinger
Copy link
Member

I am also fine with this as in basic search while with still have the back navigation issues etc. since this one fixed the search for me and so it makes sense to ship this PR with 2.0 :)

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Sep 22, 2017

👍 rebased against latest master, so from my understanding we are fine with merging this with the known open issues mentioned by @tobiasKaminsky which we postpone to > 2.0.0

Approved with PullApprove

@tobiasKaminsky
Copy link
Member

If @mario is fine with this approach, I still have to review/test it, so no merging right now please

@mario
Copy link
Contributor Author

mario commented Sep 22, 2017 via email

@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Sep 22, 2017

Tested on regular nc 12 and also on nc12 with ldap
👍

Approved with PullApprove

@AndyScherzinger
Copy link
Member

Woohoo, so Rebase and Merge 🚀

Signed-off-by: Mario Danic <mario@lovelyhq.com>
@tobiasKaminsky tobiasKaminsky merged commit 185cdce into master Sep 22, 2017
@tobiasKaminsky tobiasKaminsky deleted the fix-search-issues branch September 22, 2017 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Photos & Favs Won't Load

4 participants