add possibility to use QuantityStrings for the toolbar #39

hardysim · 2017-01-31T09:57:23Z

hardysim

I have a problem here:

I've copied setTitles() from the builder and overloaded it with my quantity-string-res. It's working fine but only if the caller (sample-app) defines both setTitles()-methods.

If not (just using the new quantity-one), the list is empty and the app runs into an Exception when Builder#build() is reached:

rxjava's SafeSubscriber#onNext()

java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String java.lang.String.trim()' on a null object reference

Any idea?

hardysim · 2017-01-31T10:05:04Z

...e/src/main/java/com/davidecirillo/multichoicesample/sampleToolbar/SampleToolbarActivity.java

@@ -90,7 +90,8 @@ private void setUpAdapter(List<String> strings) {

        MultiChoiceToolbar multiChoiceToolbar =
                new MultiChoiceToolbar.Builder(SampleToolbarActivity.this, toolbar)
-                        .setTitles(toolbarTitle(), "item selected")
+                        .setTitles(toolbarTitle(), "item selected") // TODO crashing when removed !?


Can't remove this. Otherwise the list is empty?!

Looks like there is an issue with the way i'm populating the data with RXjava check out that piece of code, it's just generating some items.

dvdciri · 2017-01-31T10:04:26Z

...erview/src/main/java/com/davidecirillo/multichoicerecyclerview/MultiChoiceToolbarHelper.java

@@ -31,13 +33,13 @@
        }
    }

-    void updateToolbar(int itemNumber) throws IllegalStateException {
-        if (itemNumber == ZERO) {
+    void updateToolbar(int selectedItemCount) throws IllegalStateException {


No need to change the name.

The variable-name wasn't clear to be so I changed it. I think this is clearer now.

I can revert it, if you like. Should I?

dvdciri · 2017-01-31T10:06:38Z

...e/src/main/java/com/davidecirillo/multichoicesample/sampleToolbar/SampleToolbarActivity.java

@@ -90,7 +90,8 @@ private void setUpAdapter(List<String> strings) {

        MultiChoiceToolbar multiChoiceToolbar =
                new MultiChoiceToolbar.Builder(SampleToolbarActivity.this, toolbar)
-                        .setTitles(toolbarTitle(), "item selected")
+                        .setTitles(toolbarTitle(), "item selected") // TODO crashing when removed !?


Just keep the plural, remove the other one.

Yeah - tried that of course but then it isn't working. When only keeping the plural-setTitles() I get an exception and the list is empty (no adapter added). See comment above.

dvdciri · 2017-01-31T10:10:28Z

Please bump the release version accordingly to the changes done.
Can't help you at the moment with the issue, make sure everything is working fine and tested manually and unit test.

hardysim · 2017-01-31T10:36:22Z

Everything seems fine and working now.

Please bump the release version accordingly to the changes done

@dvdciri What do you mean? Should I increase PUBLISH_VERSION in build.gradle to 2.1.0?

dvdciri · 2017-01-31T12:19:38Z

Yes, exactly also, squash everything into a single commit with testing and stuff and i don't know why i can't see the test running on this PR, can you create a branch under my repo in feature/branch_name? Not sure if have the access though

Provide enough info on the commit message

dvdciri · 2017-01-31T12:22:46Z

sample/src/androidTest/java/com/davidecirillo/multichoicesample/SampleToolbarActivityTest.java

@@ -147,7 +147,7 @@ public void testMultiChoiceToolbar() {
                .perform(RecyclerViewActions.actionOnItemAtPosition(0, longClick()));

        onView(allOf(isAssignableFrom(TextView.class), withParent(isAssignableFrom(Toolbar.class))))
-                .check(matches(withText(containsString("1"))));
+                .check(matches(withText(containsString("One"))));


You shouldn't change this test because this was checking that the item number has changed to the correct, create some new test, with the specific plural scenarios about the feature that you added and make sure al the tests instrumental and unit are passing!

dvdciri · 2017-01-31T12:22:56Z

sample/src/androidTest/java/com/davidecirillo/multichoicesample/SampleToolbarActivityTest.java

@@ -238,7 +238,7 @@ public void testSingleClickModeOptionMenu() {

        //Check the toolbar as well
        onView(allOf(isAssignableFrom(TextView.class), withParent(isAssignableFrom(Toolbar.class))))
-                .check(matches(withText(containsString("1"))));
+                .check(matches(withText(containsString("One"))));


hardysim · 2017-01-31T13:20:35Z

Sry, didn't check all tests. Should be fine now.

I've add a switch to SampleToolbarActivity to use different setTitles()-methods and added SampleToolbarActivityPluralsTest to test the new plural-toolbar.

Something left now?

hardysim · 2017-01-31T13:26:59Z

squash everything into a single commit with testing and stuff

You mean to have only one (merge-) commit with all changes in your history once merged? Can't you rebase right here from the PR?

If not, what's the best practice to get this work? Merge into my fork and use a cherry-pick on the merge-commit for the PR into your project? Wouldn't this result in a new PR (and close of this one)?

i don't know why i can't see the test running on this PR

Already wondered. Do you have a check for the branch-name (like "starts with feature/")?

can you create a branch under my repo in feature/branch_name

No, it seems like I don't have enough permissions for that

dvdciri · 2017-01-31T13:45:03Z

Nope, no check for branch name, is not running because is not a branch under my repo.. weird.
Anyway please do the following:

We don't need to change the sample, so everything related to it just keep it in the same way, the changes must be retro-compatible so please, keep the sample as is with the normale setTitle() but make sure you check all the cases with both, on or the other, run test and add test for that specific scenario (you did).
I can squash when i merge

So everything under the sample package, a part from the instrumental testing should not be touched , try to keep everything as simple as possible
I'll review everythin again once you finished

hardysim · 2017-01-31T13:52:02Z

The changes I made to the sample are only needed to get an activity using the new method. As I did not replaced the original .setTitles() everything is retro-compatible.

So, what do you want? An additional Activity using the new plural-setTitles() (which I can use for my UI-test)?

dvdciri · 2017-01-31T14:03:30Z

The it's fine. I'll review everything again later on. We need to find a way to run CircleCI on it.

hardysim · 2017-01-31T14:08:59Z

If I get it right, CircleCI should run checks for PR of a fork automatically. But yes, it's obviously not working.

https://circleci.com/docs/fork-pr-builds/

I found this - seems like it's possible that CircleCI doesn't recognize this project as OpenSource !?

dvdciri · 2017-02-01T11:14:25Z

Yeah looks like is not recognising it as OpenSource but we need to run CircleCI on it before i can merge it.

hardysim · 2017-02-01T14:20:32Z

I'm not familiar with CircleCI and cannot help you with that - maybe create a bug on their tracker?

hardysim · 2017-02-01T16:16:27Z

I got an answer from @ ecgreb (lostzen/lost#94 (comment)):

try under the Circle web UI Settings > Advanced Settings make sure "Free and Open Source" and "Build forked pull requests" are both enabled.

@dvdciri can you check that?

dvdciri · 2017-02-01T16:19:36Z

Done, enabled it! Let's see

hardysim · 2017-02-01T16:23:40Z

Do we need to do an other push to this branch to get it triggered?

dvdciri · 2017-02-01T16:24:41Z

Can you do an interactive git squash into a single commit? so we trigger the build?

…g items

hardysim · 2017-02-02T08:42:43Z

So it looks running now :)

But I'm not happy with the way I squashed the commits. I used the workflow from http://stackoverflow.com/a/25357146 which worked fine (other than rebase, which results in one conflict after an other) but I needed to force-push.

Is there a non-rebase, non-force-push way to squash a whole branch into one commit to contribute to a foreign project (just like this case here)?

I think the squash-merge in this very PR here would do the trick, right?

dvdciri · 2017-02-02T08:47:07Z

That's the only way, you have conflict because is rewriting the history and adding one commit in the other on top of the branch you reabsed.

hardysim · 2017-02-02T08:49:34Z

That's the only way, you have conflict because is rewriting the history and adding one commit in the other on top of the branch you rebased.

Yeah, think so. But just to make sure I get it right: Squash-Merging a PR in github would work too, right?

https://help.github.com/articles/configuring-commit-squashing-for-pull-requests/

dvdciri · 2017-02-02T08:52:31Z

Yes, correct!

I would just do one change, don't add the possibility to change the mode via UI through the option menu, just keep the logic for the test but remove the changes done to the menu, option menu clicked and what need to change accordingly.
So in the sample will always be plurals but for test we can have different.

Other that that LGTM!

hardysim · 2017-02-02T08:57:31Z

So in the sample will always be plurals but for test we can have different.

That won't work as your unit-tests rely on the toolbar showing "1", remember? So I need to leave SampleToolbarActivity as it is and cannot use the plurals.

dvdciri · 2017-02-02T09:00:03Z

Right.. then just show a toast in the Case that the use clicks on the option menu explaining what he've just enabled/disabled, cause ATM you're just doing the logic bug at least a feedback for the user is needed

hardysim · 2017-02-02T10:41:06Z

All done. Ready to merge?

PS: Will you create a new release so I can use this new feature right now? ;-)

dvdciri · 2017-02-02T11:10:38Z

Not sure when i'm gonna do a release, but will definitely happen sometimes soon. Thanks

hardysim commented Jan 31, 2017

View reviewed changes

dvdciri reviewed Jan 31, 2017

View reviewed changes

add possibility to use QuantityStrings for the toolbar while selectin…

29ef037

…g items

add toast when user changes to QuantityMode.PLURALS in toolbar-sample

1fe3568

hardysim changed the title ~~WIP add possibility to use QuantityStrings for the toolbar~~ add possibility to use QuantityStrings for the toolbar Feb 2, 2017

dvdciri merged commit d9be4ce into dvdciri:develop Feb 2, 2017

hardysim deleted the 33-QuantityStrings branch February 2, 2017 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add possibility to use QuantityStrings for the toolbar #39

add possibility to use QuantityStrings for the toolbar #39

hardysim commented Jan 31, 2017 •

edited

hardysim left a comment

hardysim Jan 31, 2017

dvdciri Jan 31, 2017

dvdciri Jan 31, 2017

hardysim Jan 31, 2017

dvdciri Jan 31, 2017

hardysim Jan 31, 2017

dvdciri commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017 •

edited

dvdciri Jan 31, 2017

dvdciri Jan 31, 2017

hardysim commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017 •

edited

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 1, 2017

hardysim commented Feb 1, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 1, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

add possibility to use QuantityStrings for the toolbar #39

add possibility to use QuantityStrings for the toolbar #39

Conversation

hardysim commented Jan 31, 2017 • edited

hardysim left a 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

dvdciri commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardysim commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017 • edited

hardysim commented Jan 31, 2017

dvdciri commented Jan 31, 2017

hardysim commented Jan 31, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 1, 2017

hardysim commented Feb 1, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 1, 2017

dvdciri commented Feb 1, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Feb 2, 2017

dvdciri commented Feb 2, 2017

hardysim commented Jan 31, 2017 •

edited

dvdciri commented Jan 31, 2017 •

edited

dvdciri commented Jan 31, 2017 •

edited