Skip to content
This repository was archived by the owner on Jan 23, 2024. It is now read-only.

added notification with N style (and add android O channel notification )#791

Open
MARTINI1 wants to merge 9 commits intofarkam135:masterfrom
MARTINI1:notif-goiv
Open

added notification with N style (and add android O channel notification )#791
MARTINI1 wants to merge 9 commits intofarkam135:masterfrom
MARTINI1:notif-goiv

Conversation

@MARTINI1
Copy link
Contributor

@MARTINI1 MARTINI1 commented Oct 14, 2017

Android 4.4 kitkat
not-kitkat-goiv

Android 7 nougat
not-nougat-goiv

Android 8 oreo
not-oreo-goiv

**
I have been forced to change the layout to relative only in the compact notificiations since by the change to the N style they appeared wrong in android >=7 and with the done change already appear correctly (as in the screenshots)

@harkin
Copy link
Contributor

harkin commented Oct 14, 2017

The linearlayout -> relativelayout change is probably going to make it less performant, not more. Probably for the layouts with three nested linearlayouts, definitely for the one without that level of nesting.

@thearaks
Copy link
Collaborator

Thanks MARTINI1!
The end result looks very good on Android N.
Have you tried on older platforms to see if the changes brakes something? (KitKat, Lollipop, Marshmallow)
Inside NotificationManager you added an if statement and duplicated a lot of code (just to add some properties to the builder)... This is not very good, you should only change the attributes specific to N in the if statement, not copy-paste all the notification building code.
Also, you revised all the layouts to achieve the desired effect on N. This looks to be unnecessary if you just wanted to change the vertical padding on N (or, maybe, I'm missing other differences but given the many changes I can't say it for sure). Are you sure all these layout changes are strictly needed?

As a side note, there's just one problem with this PR and others you made before: you need to try to make more atomic changes (and smaller when possible).
This helps code review and also reverting to previous versions of the code when something gets broken.
As a general rule, you should try to make more smaller PRs instead of a single big one, and in each PR you should try to split atomic changes in different commits! ;)

@MARTINI1
Copy link
Contributor Author

@harkin relativelayout is more effective, and they say it here:

Sticking to the basic features is unfortunately not the most efficient way to create user interfaces. A common example is the abuse of LinearLayout, which leads to a proliferation of views in the view hierarchy. Every view — or worse, every layout manager — that you add to your application comes at a cost: initialization, layout and drawing become slower. The layout pass can be especially expensive when you nest several LinearLayout that use the weight parameter, which requires the child to be measured twice.

and here:

It is a common misconception that using the basic layout structures leads to the most efficient layouts. However, each widget and layout you add to your application requires initialization, layout, and drawing. For example, using nested instances of LinearLayout can lead to an excessively deep view hierarchy. Furthermore, nesting several instances of LinearLayout that use the layout_weight parameter can be especially expensive as each child needs to be measured twice. This is particularly important when the layout is inflated repeatedly, such as when used in a ListView or GridView.

*stackoverflow

I wanted to fix the strange trick of making negative margins and implementing the nougat style to the notification

this last forced me to edit the layouts and since it was as I have transformed them to relativelayout for a better, cleaner solution and avoiding doing so many linearlayout

@thearaks
Copy link
Collaborator

That's a general rule, performances depends on every case. Anyway with layouts so small there's no much difference nowadays that smartphones are so powerful.

Regarding this, you should make 2 PRs:
One that switch from linearlayout to relatively
And one that adds N support

This make code revisions much more readable!

@MARTINI1
Copy link
Contributor Author

MARTINI1 commented Oct 14, 2017

@thearaks yes, tested in VM kitkat and oreo, and my mobile with nougat (watch the captures please xD )

about lot of code, you are right, what I did not find how to make a smart change apart I had (and I have) a bug or something missing, that when I put the setStyle tells me that I did not find reference to that when I copied it from the official documentation (and I worked before but was mixed with another PR and doing so clean I was not going :()

if you have any suggestion tell me please, I will be very grateful because I am not completely satisfied

@harkin
Copy link
Contributor

harkin commented Oct 14, 2017

@MARTINI1 I'm not sure I'd put much stock in a blog post from 2009 tbh, a lot has changed since the donut/eclair days. Those layouts aren't 'excessively deep' and the last link to stackoverflow is the most important I think. Relativelayouts always, unconditionally, causing a second measure pass is normally slower than when you get from the linearlayouts (there's only one using a weight there that I see). If the change makes it easier to do the new notification styles, great!

Re lots of code, both notifications being created in code look very similar, and it happens twice. Maybe a method could be refactored out there that generates the similar notifications? For the XML, looks like many states share similar views, maybe some commonalities could be extracted and included into these layouts (not sure this is worth it TBH)?

@thearaks
Copy link
Collaborator

Aside the duplicate code in GoIVNotificationManager that could easily be fixed (and is unnecessary, since the support library handles the platform difference anyway. But we need support library version 26 and also we should update out target API level to 26),
I'm not sure that the buttons alignment to the right side is correct.
I used the negative margin (wich are perfectly fine with LinearLayouts) of -12dp to remove the white space inside the 48dp icon button because it contains a 24dp image.
So (48 - 24) / 2 = 12.
The button size was set to 48dp to ensure it had the minimum touch area as specified in Android guidelines, because otherwise it would be difficult to touch.
The negative margin was the only way to mantain its size while alining its visibile (drawing) area to the other notifications right keyline.

Also, in general, the new proposed layouts are less readable than the previous version.

(I'll post an updated version of this PR to show how the same changes can be implemented more cleanly)

Ufortunately, this PR is a little too "dirt". IMHO I wouldn't merge this at the moment.
Please wait for further review, thank you!

@MARTINI1 MARTINI1 changed the title added notification with N style and optimized layouts with relativelayout (WIP) added notification with N style and optimized layouts with relativelayout Oct 16, 2017
@MARTINI1 MARTINI1 changed the title (WIP) added notification with N style and optimized layouts with relativelayout added notification with N style (and add android O channel notification ) Oct 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants