Tablet layout #71

noelbautista91 · 2017-03-10T20:25:53Z

1 minute Demo incoming

~~There are some crashes happening with this PR, all of what I found are related to configuration changes (rotation, multiwindow, etc)~~ Crashes related to rotation and configuration changes are squashed! Please report if you find any

Please feel free to comment/modify to this PR whatever it needs to be ready for production!

I would rather have us fix all related issues before merging in this PR. it's a good size and can potentially introduce a lot of issues. On the surface this PR seems to change a lot of classes, but a good number of them is just modifying layouts, menu xmls, renaming some ids, etc to enable tablet layout.

I've added UI tests for tablet ~~(in a separate test). You could probably annotate this test to only run on tablet layouts.~~ Given the similarities of phone vs tablet layout, they are packed into one suite of tests.

Syncing seems to work fine, thanks to the well abstracted data store classes :D

TODO:

Test different scenarios for orientation changes

More photos:

deftelf · 2017-03-14T00:00:54Z

app/src/androidTest/java/org/standardnotes/notes/StarterActivityTest.java

+        } else {
+            // Not sure why adding this delay works, not a good solution.
+            try {
+                Thread.sleep(500);


Oooooooo....

(but it's not the end of the world in a test)

I was thinking it might be some animation stuff, but I haven't dug deep into what makes this work.

deftelf · 2017-03-14T00:03:29Z

app/src/main/java/org/standardnotes/notes/MainActivity.kt

+        bundle.putString(NoteListFragment.EXTRA_NOTE_ID, uuid)
+        noteFragment!!.arguments = bundle
+        if (findViewById(R.id.note_container) == null) {
+            removeDrawerToggle()


I see why you went for the single-activity solution, but it does create weirdness around the action bar. Feel like there must be a better solution than this.

I agree about the weirdness. but isn't using NoteActivity beating the purpose of the master-detail pattern?

I also think this implementation is weird. I wish I had better ideas

Possible solution - push the setup into NoteListFragment.setupActionBar(ActionBar, Drawer) and NoteFragment.setupActionBar(ActionBar, Drawer) methods, and have them do what is currently in those *DrawerToggle() methods. Then whenever the fragment in note_list_container changes, call its setupAcitonBar() and have it do whatever changes needed.

note_list_container probably needs renaming to primary_container or something like that.

You also probably want to keep the back arrow when it's in the single_pane mode and showing a note, and have it do the right thing. Possibly those fragments need to have a onBackPressed() method too.

deftelf · 2017-03-14T00:05:27Z

app/src/main/java/org/standardnotes/notes/frag/NoteFragment.kt


        val noteUuid =
                savedInstanceState?.getString(NoteListFragment.EXTRA_NOTE_ID) ?:
                        arguments?.getString(NoteListFragment.EXTRA_NOTE_ID)
-        if (noteUuid != null) {
-            note = SApplication.instance.noteStore.getNote(noteUuid)!!
+        if (noteUuid != "") {


This doesn't protect against null

deftelf · 2017-03-14T00:06:15Z

app/src/main/java/org/standardnotes/notes/frag/NoteFragment.kt

-        if (noteUuid != null) {
-            note = SApplication.instance.noteStore.getNote(noteUuid)!!
+        if (noteUuid != "") {
+            note = SApplication.instance.noteStore.getNote(noteUuid!!)!!


See above, which is why you now need the !! I-love-nulls command. Kotlin is clever!

deftelf · 2017-03-14T00:10:19Z

app/src/androidTest/java/org/standardnotes/notes/TestHelper.java

+        };
+    }
+
+    public static boolean isScreenSw600dpAndLandscape(Activity activity) {


So I don't like that the normal code references a TestHelper - this probably wants moving into a separate class. The other problem is that this duplicates the resource sw600-land config, which is fine for tests but I'm lairy about using it in production. If that is changed and not this, we have a bug in the future, or if this doesn't accurately reflect what the resource config does, and I'm not 100% sure it will.

what do you mean? this class only exists in the androidTest folder, making it only available to ui tests. it's only used in the tests.

Sorry, I'm mis-reading the code. Ignore this!

deftelf · 2017-03-14T00:13:10Z

app/src/main/res/values-sw600dp-land/refs.xml

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>


Is this the right config do we think? Maybe just values-w600dp, which would also pick up very large portrait screens, which is fine.

Why is this in a values ref rather than just directly having the two layouts in layout/ and layout-w600dp/ ?

Is this the right config

I agree, I played with just sw600dp while writing this feature and it looked fine to me.

I used refs since I wanted to name the "normal" layout and the "tablet" layout something generic (two_pane and single_pane). Although it doesn't really matter in the context of this app since it's simple in terms of UI.

w600dp != sw600dp, sw is constant shortest width regardless of orientation , w is the current width.

Oh yeah. Good to know, I'll make that change when I have time

noelbautista91 · 2017-03-15T05:38:26Z

@deftelf Implemented some of your comments, I have yet to try out the solution you suggested for the action bar stuff.

Changing to w600dp now shows the "tablet" layout for a phablet (Pixel XL) in landscape mode.

This broke our tests (just for phablets in landscape mode), I think because "phones" do not present the home screen / menu in landscape the way tablet does. If you run the test suite, you'd see that whenever the stuff in @Before is called, the app actually starts up another instance of itself in portrait mode and then rotates to landscape. This adds a weird delay in our test suite, making it fail.

Not sure what a proper solution is (but I was considering making the test wait.. D:) But maybe our tests need some refactoring. Maybe we don't need to have the signin stuff in Before and the logout stuff in @After. Instead we could just call these manually at the beginning and end of each test (tagSomething, createNote, etc).

This way, the app wouldn't have to create a new instance of itself for each test. The tests would each have to logout and bring itself back to the LoginActivity by calling logout() at the end of each test instead of using @After. They would also have to begin with signin() instead having signin() in @Before, and move the line that starts an instance of the StarterActivity to somewhere else that gets called only when the StarterActivityTest is run.

There is a way to make Espresso only call @Before and @After only before and after the start of the test class, instead of each test method. I propose we do that and have our IdlingResource register and unregister there, as well as starting StarterActivity in @Before.

* Move fab from activity to fragment * Make tests more lenient, some tests we checking for unnecessary criteria like withParent, when we're guaranteed to have only one instance of a certain view per screen (like fab, list)

* Rename some things (list, fab) because these names are used in different places but refer to different things. This can confuse people and intellij when refactoring or renaming. * Moving the fab (for adding new notes) to a menu item for tablets makes sense from a UI perspective. The fab would be either on the note list (on the left side) or on top of the note fragment (on the right). The first option doesn't make sense, because the fab is supposed to be accessible. The second option doesn't make sense because it covers the note fragment. My compromise is to add it as a menu item when space permits.

* Check for null noteUuid * Use w600dp instead of sw600dp-land * Rename note list and note containers to master and detail containers * Broke e2e test for a landscaped phablet. The way the tests are set up: When re-launching the app, the app starts in portrait mode and rotates after a split second. I think this is because for "phones", Android does not allow landscape mode in the menu. Tests in portrait and in actual tablet layout should work fine.

noelbautista91 · 2017-03-22T15:23:31Z

Just rebased this branch the latest code.

I played around with the test last night to get it to pass on a Pixel XL emulator on landscape, but to no success. If anyone would wanna take a stab at it please feel free. Tests pass on portrait mode for phones, and in landscape mode for tablets. Doesn't seem to pass on landscape modes for phablets.

What would get this feature to be merged? It looks pretty ready, maybe some more real world testing.

deftelf · 2017-03-22T16:30:01Z

Looking good. I would like to get the encryption change out first this weekend as a 1.0 release, then probably load this and test it over the week and put out as 1.1

noelbautista91 mentioned this pull request Mar 11, 2017

Tablet layout #54

Open

deftelf reviewed Mar 14, 2017

View reviewed changes

noelbautista91 mentioned this pull request Mar 14, 2017

Arch change - use event mechanism to synchronise fragments #66

Closed

deftelf mentioned this pull request Mar 14, 2017

When a change comes in for the currently opened note, should update text view #48

Closed

noelbautista91 added 8 commits March 21, 2017 21:17

Initial pass at tablet layout

bbf9a5c

* Move fab from activity to fragment * Make tests more lenient, some tests we checking for unnecessary criteria like withParent, when we're guaranteed to have only one instance of a certain view per screen (like fab, list)

Fix crashes when using multiwindow or rotating

959d1d4

Disable drawer toggle when showing NoteFragment on vertical layout

d57148f

Properly load list and note fragments during rotation

3c21a86

Fix note title and menu items

43e8600

Clean up code

6a7eddd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tablet layout #71

Tablet layout #71

noelbautista91 commented Mar 10, 2017 •

edited

Loading

deftelf Mar 14, 2017

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017

deftelf Mar 14, 2017

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017

deftelf Mar 14, 2017

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017

deftelf Mar 14, 2017

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017 •

edited

Loading

deftelf Mar 14, 2017

noelbautista91 Mar 14, 2017

noelbautista91 commented Mar 15, 2017 •

edited

Loading

noelbautista91 commented Mar 22, 2017

deftelf commented Mar 22, 2017

Tablet layout #71

Are you sure you want to change the base?

Tablet layout #71

Conversation

noelbautista91 commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelbautista91 Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelbautista91 commented Mar 15, 2017 • edited Loading

noelbautista91 commented Mar 22, 2017

deftelf commented Mar 22, 2017

noelbautista91 commented Mar 10, 2017 •

edited

Loading

noelbautista91 Mar 14, 2017 •

edited

Loading

noelbautista91 commented Mar 15, 2017 •

edited

Loading