Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: offer to merge margins/padding when possible, into horizontal/vertical/all #16

Closed
AndroidDeveloperLB opened this issue Feb 10, 2022 · 25 comments
Assignees
Labels
enhancement New feature or request

Comments

@AndroidDeveloperLB
Copy link

Meaning in layout XML file, if there is android:layout_marginLeft="20dp" android:layout_marginRight="20dp" , offer android:layout_marginHorizontal="20dp".

Same goes for android:layout_marginStart="20dp" android:layout_marginEnd="20dp" to offer android:layout_marginHorizontal="20dp".

And of course for vertical ones, from android:layout_marginTop="20dp" android:layout_marginBottom="20dp" into android:layout_marginVertical="20dp" .

And the very rare case of all sides that are the same, from android:layout_marginLeft="20dp" android:layout_marginRight="20dp" android:layout_marginTop="20dp" android:layout_marginBottom="20dp" into just android:layout_margin="20dp" . And the same if used with start&end.

Of course, if there is a mix that contradicts the merging, don't offer it. For example android:layout_marginLeft="20dp" android:layout_marginStart="15dp" . However, if there is a mix that is identical (start&end&left&right with the same value, for example), offer to merge.

Same goes for padding.

If the mix is weird, I suggest to warn about it, as a different inspection.

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Feb 10, 2022

Note that in styles file, sadly layout_marginHorizontal (and the others) aren't allowed if the minApi is less than 26.
In this case, don't offer to merge unless the minApi is at least 26.

@Miha-x64
Copy link
Owner

Yep, nice idea!
The problem is that these new horizontal/vertical attributes work in some strange way: with minSdk 21, you will end up having two layout files from one XML, default and v22, and default one would not contain these indents at all. Surprisingly, this works on SDK 21. So I need some more research to figure out for which minSdk this should be offered.

@Miha-x64 Miha-x64 added the enhancement New feature or request label Feb 10, 2022
@Miha-x64 Miha-x64 self-assigned this Feb 10, 2022
@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Feb 10, 2022

As I remember, this is possible on old versions of Android thanks to android-x.
So if you want to be on the safe side, offer it only for minSdk that's large enough. Try without using android-x to make sure.

@Miha-x64
Copy link
Owner

Not really, this is AAPT's job: https://android.googlesource.com/platform/frameworks/base/+/master/tools/aapt2/readme.md#version-2_16

The safest way is to go with minSdk 26 but who will use it?..

@Miha-x64
Copy link
Owner

I think we should go with minSdk 22.

image

image

@AndroidDeveloperLB
Copy link
Author

OK so I forgot how it worked. Sorry.
Please try the lowest API possible and make sure that indeed it works fine (run on emulator).

You can also have an inspection for weird cases, such as layout_marginHorizontal being set and yet also android:layout_marginStart was set, especially if they are of different values.

@Miha-x64
Copy link
Owner

Agree.
Can I ask you to do a little research? I know that padding overrides padding(Left|Top|Right|Bottom), and same applies for margin. What about Start|End, and Horizontal|Vertical? Can you build a list of priorities, please?

@AndroidDeveloperLB
Copy link
Author

What do you mean?
If you talk about the mixes, I remember that I've found an inconsistency between how it works on padding, vs how it works for margins. That's why I suggest to avoid handling weird mixes except just warn about it.

@Miha-x64
Copy link
Owner

The question is how to warn. For example, I could grey out an attribute and say “paddingLeft is overridden by padding”

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Feb 10, 2022

I would prefer to be strict about it, not allowing such mixes, and the warning would say about it in general.
The reason for being strict is because of the confusion that can come from those mixes, especially as padding works differently than margins (I think for one of them the priority of "left" doesn't get applied).

Of course, you can make this inspection optional

@Miha-x64
Copy link
Owner

Done!

@AndroidDeveloperLB
Copy link
Author

What's done? Everything? Padding, margins...?
What did you do about the weird cases?

@Miha-x64
Copy link
Owner

Reporting on attributes which could or will be overridden by others.

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Feb 23, 2022

I don't understand. Are you talking now about the mixed, weird cases?

@Miha-x64
Copy link
Owner

Oh, I've forgot about merging 😟

@Miha-x64 Miha-x64 reopened this Feb 23, 2022
@AndroidDeveloperLB
Copy link
Author

Merging was the original idea. The mixed cases are a special case of it...

@AndroidDeveloperLB
Copy link
Author

Was it now added?

@Miha-x64
Copy link
Owner

Miha-x64 commented May 8, 2022

image

@AndroidDeveloperLB
Copy link
Author

Thank you!

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Jun 26, 2022

@Miha-x64 For some reason I can't find this inspection (v0.24). How is it called?

Here's what I see when I search "Mike":

image

@Miha-x64
Copy link
Owner

@AndroidDeveloperLB
Android | Useless resource element

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Jun 26, 2022

@Miha-x64 It doesn't talk about padding/margins:

"Reports useless elements of XML Drawable resources, such as single-item layer-lists, insetless insets, empty shapes, and vector elements: empty paths and clip-paths, invisible paths, useless clip-paths and groups, attributes with no effect."

And, after analyzing, I don't see even one example that it offers to merge, even though there should be...
I see only of : "subpixel precision", "invisible sub-path", "The path is fully overdrawn".

Are you sure that's correct?

@AndroidDeveloperLB
Copy link
Author

@Miha-x64 Speaking of "The path is fully overdrawn", here:
#29

@Miha-x64
Copy link
Owner

@AndroidDeveloperLB, I will extend inspection description to mention all its features.

I don't see even one example that it offers to merge, even though there should be...

Please submit a separate issue with a reproducer.

@AndroidDeveloperLB
Copy link
Author

@Miha-x64 OK here:

#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants