Add builder API for applying WindowInsets #62

poliver · 2020-06-05T05:10:39Z

For this most part, this just wraps the existing functionality with a builder API. Initial motivation for this was Insetter not providing a way to consume insets after applying them to a given View. A potential result is insets being applied a second time by child Views/ViewGroups that automatically handle insets internally. I've restricted the scope of the original static utils where possible, and I also added a new consumeSystemWindowInsets attribute to the binding adapter.

poliver · 2020-06-05T05:14:10Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+            if (consumeSystemWindowInsets) {
+              return insets.consumeSystemWindowInsets();
+            } else {
+              return insets;
+            }


I also considered surfacing this so callers can be more granular with how they consume insets. Consuming everything might be overkill, but it solves the initial issue described in the PR description.

In that case, would it make sense to have OnApplyInsetsListener.onApplyInsets return WindowInsetsCompat? I could alternatively add a OnConsumeInsetsListener interface.

This also seems like it would make testing Builder.consumeSystemWindowInsets a bit easier

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

poliver · 2020-06-05T05:22:30Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+    public void applyToView(@NonNull View view) {
+      build().setOnApplyInsetsListener(view);
+    }


Not sure if this needs to be part of the builder

I like this. I'd make the doc a bit clearer though. Something like:

Builds the Insetter instance and sets it as a OnApplyWindowInsetsListener on [view].

Might also be worth returning the built Insetter from the function too.

chrisbanes · 2020-06-05T08:27:08Z

Looks great to me overall, a much nicer API for devs. Thank you 🙌

While looking through this some of the old API looks pretty crufty. While we're making big API changes, we may as well fix everything in one release 😃

What do we think about the use of EnumSet? While they have an optimal implementation, the API is not so nice. What do you think of moving: @Nullable EnumSet<InsetDimension> dimensions to InsetDimension... dimensions?
The enum InsetDimension name is a bit long and not particularly clear. How about a rename to Insetter.Side? Feels clearer to me.

chrisbanes · 2020-06-05T08:11:24Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+     * @see Insetter#setOnApplyInsetsListener(View)
+     */
+    @NonNull
+    public Builder setOnApplyInsetsListener(OnApplyInsetsListener onApplyInsetsListener) {


@Nullable param?

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

chrisbanes · 2020-06-05T08:13:36Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+            if (consumeSystemWindowInsets) {
+              return insets.consumeSystemWindowInsets();
+            } else {
+              return insets;
+            }


chrisbanes · 2020-06-05T08:14:04Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+    }
+  }
+
+  public static Builder builder() {


chrisbanes · 2020-06-05T08:15:57Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+    public void applyToView(@NonNull View view) {
+      build().setOnApplyInsetsListener(view);
+    }


I like this. I'd make the doc a bit clearer though. Something like:

Builds the Insetter instance and sets it as a OnApplyWindowInsetsListener on [view].

Might also be worth returning the built Insetter from the function too.

chrisbanes · 2020-06-05T08:16:43Z

library/src/main/java/dev/chrisbanes/insetter/Insetter.java

+     * @see Insetter#setOnApplyInsetsListener(View)
+     */
+    @NonNull
+    public Builder consumeSystemWindowInsets(boolean consumeSystemWindowInsets) {


We should mention the default value of this in the doc.

poliver · 2020-06-05T20:44:15Z

What do we think about the use of EnumSet? While they have an optimal implementation, the API is not so nice. What do you think of moving: @Nullable EnumSet<InsetDimension> dimensions to InsetDimension... dimensions?

The enum InsetDimension name is a bit long and not particularly clear. How about a rename to Insetter.Side? Feels clearer to me.

Renaming to Insetter.Side makes sense. I've added that change. But the change from EnumSet to vararg is adding some complexity between the builder API, the library widgets, etc. What do you think of a Side.Builder() instead?

...src/main/java/dev/chrisbanes/insetter/widgets/constraintlayout/InsetterConstraintHelper.java

ZacSweers · 2020-06-06T22:22:53Z

Really like how the bit field approach turned out 👍

chrisbanes

Really liking where this is heading. Just a few more tweaks and we can

chrisbanes · 2020-06-07T08:16:10Z

library/src/main/java/dev/chrisbanes/insetter/SideUtils.java

+public final class SideUtils {
+
+  @Sides
+  public static int create(boolean left, boolean top, boolean right, boolean bottom) {


Looks great. Maybe move this to the Side class? Feels more fundamental than being in a utils class.

chrisbanes · 2020-06-07T08:17:14Z

library/src/main/java/dev/chrisbanes/insetter/Side.java

-import android.view.View;
-import androidx.core.view.WindowInsetsCompat;
-import java.util.EnumSet;
+public final class Side {


Let's add a NONE = 0 too, rather than relying on 0 in the Insetter logic.

...src/main/java/dev/chrisbanes/insetter/widgets/constraintlayout/InsetterConstraintHelper.java

chrisbanes · 2020-06-07T20:48:16Z

library/src/main/java/dev/chrisbanes/insetter/SideUtils.java

@@ -24,14 +24,6 @@
 */
 public final class SideUtils {


Sorry, one last thing. We can now make this pkg-private.

chrisbanes

Perfect, thanks for the contribution!

Phil Oliver added 13 commits June 4, 2020 16:44

Add builder

39dac62

Remove deprecated applyInsetsToView method

db9b3f0

Add inset apply and consume logic

0f632a3

Nullable dimensions

ec990b7

Initial feedback

befc107

Update binding adapter

ca7e4d2

Update ktx

2a89633

Add consume option to extensions

adfabf9

Add binding for consumeSystemWindowInsets

cde6ac5

Clean up builder and update tests

209ab1f

Remove Method.java

8fc3427

Add some docs

21a0528

Revert newline

86b0def

googlebot added the cla: yes label Jun 5, 2020

poliver commented Jun 5, 2020

View reviewed changes

library/src/main/java/dev/chrisbanes/insetter/Insetter.java Show resolved Hide resolved

poliver commented Jun 5, 2020

View reviewed changes

library/src/main/java/dev/chrisbanes/insetter/Insetter.java Show resolved Hide resolved

poliver commented Jun 5, 2020

View reviewed changes

Phil Oliver added 2 commits June 4, 2020 22:31

Spotless

5ee4705

Spotless

2d086a0

chrisbanes reviewed Jun 5, 2020

View reviewed changes

Phil Oliver added 2 commits June 5, 2020 10:31

Initial PR feedback

d529b3b

Rename InsetDimension to Insetter.Side

51cc8fc

Phil Oliver added 5 commits June 5, 2020 14:29

Nonnull Side EnumSets

3896ee5

Forgot this doc

05beda0

Convert EnunSet<Side> to Sides

0e51764

Remove "dimension" naming from widgets to disambiguate ViewDimensions

c226ed5

Conver Sides to int

b7ab205

Phil Oliver added 2 commits June 5, 2020 19:44

Rename to SideUtils

b0e5e16

Organize some files

11cef64

poliver commented Jun 6, 2020

View reviewed changes

...src/main/java/dev/chrisbanes/insetter/widgets/constraintlayout/InsetterConstraintHelper.java Outdated Show resolved Hide resolved

chrisbanes reviewed Jun 7, 2020

View reviewed changes

Add Side.NONE and Side.create

ace8afc

chrisbanes reviewed Jun 7, 2020

View reviewed changes

Phil Oliver added 3 commits June 7, 2020 14:02

int comparison instead of Object comparison

b2e8f0b

package private SlideUtils

df22d03

Add Side.ALL for convenience

eaa5dd6

chrisbanes approved these changes Jun 8, 2020

View reviewed changes

chrisbanes merged commit c570c63 into chrisbanes:master Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builder API for applying WindowInsets #62

Add builder API for applying WindowInsets #62

poliver commented Jun 5, 2020 •

edited

Loading

poliver Jun 5, 2020 •

edited

Loading

chrisbanes Jun 5, 2020

poliver Jun 5, 2020 •

edited

Loading

poliver Jun 5, 2020

poliver Jun 5, 2020

chrisbanes Jun 5, 2020

chrisbanes commented Jun 5, 2020 •

edited

Loading

chrisbanes Jun 5, 2020

chrisbanes Jun 5, 2020

chrisbanes Jun 5, 2020

chrisbanes Jun 5, 2020

chrisbanes Jun 5, 2020

poliver commented Jun 5, 2020

ZacSweers commented Jun 6, 2020

chrisbanes left a comment

chrisbanes Jun 7, 2020

chrisbanes Jun 7, 2020

chrisbanes Jun 7, 2020

chrisbanes left a comment

Add builder API for applying WindowInsets #62

Add builder API for applying WindowInsets #62

Conversation

poliver commented Jun 5, 2020 • edited Loading

poliver Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poliver Jun 5, 2020 • 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

chrisbanes commented Jun 5, 2020 • 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

poliver commented Jun 5, 2020

ZacSweers commented Jun 6, 2020

chrisbanes 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

chrisbanes left a comment

Choose a reason for hiding this comment

poliver commented Jun 5, 2020 •

edited

Loading

poliver Jun 5, 2020 •

edited

Loading

poliver Jun 5, 2020 •

edited

Loading

chrisbanes commented Jun 5, 2020 •

edited

Loading