Miha-x64 / Mikes_IDEA_extensions

IntelliJ IDEA: missing parts.
Apache License 2.0
34 stars 7 forks source link

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

Closed AndroidDeveloperLB closed 2 years ago

AndroidDeveloperLB commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

AndroidDeveloperLB commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I think we should go with minSdk 22.

image

image

AndroidDeveloperLB commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

AndroidDeveloperLB commented 2 years ago

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 commented 2 years ago

Done!

AndroidDeveloperLB commented 2 years ago

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

Miha-x64 commented 2 years ago

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

AndroidDeveloperLB commented 2 years ago

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

Miha-x64 commented 2 years ago

Oh, I've forgot about merging 😟

AndroidDeveloperLB commented 2 years ago

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

AndroidDeveloperLB commented 2 years ago

Was it now added?

Miha-x64 commented 2 years ago

image

AndroidDeveloperLB commented 2 years ago

Thank you!

AndroidDeveloperLB commented 2 years ago

@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 commented 2 years ago

@AndroidDeveloperLB Android | Useless resource element

AndroidDeveloperLB commented 2 years ago

@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 commented 2 years ago

@Miha-x64 Speaking of "The path is fully overdrawn", here: https://github.com/Miha-x64/Mikes_IDEA_extensions/issues/29

Miha-x64 commented 2 years ago

@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 commented 2 years ago

@Miha-x64 OK here:

https://github.com/Miha-x64/Mikes_IDEA_extensions/issues/30