Update formatting in web_ui scene bulder to match flutter style.

[flar]

In a recent PR to add a new pushFooLayer() API to the SceneBuilder class in engine, the code format of the pushLayer methods was pointed out as not quite matching the flutter framework style. This PR is looking to find a suitable code style for these methods to embrace moving forward.

Most of the methods were formatted in this initial push to adopt the style suggested in the pushImageFilter PR, but it wasn't clear how to adapt the pushShaderMask() method to comply and I tried 2 variants here in the 2 different files for comparison and feedback.

dartfmt suggests that the fixed parameters be moved to separate lines as well, similar to pushPhysicalShape(). Note that the "one line per argument" style is not really embraced in the engine repo right now, so I hesitated on taking that suggestion to diverge even further.

The formatter built in to Android Studio with the Dart plugin also fights this style somewhat. If all of the arguments are moved to separate lines it insists on 4-space indentation which doesn't match the flutter style, but if only the named parameters (inside {}) are on their own lines then it seems perfectly happy with 2-space indentation.

Also, currently the main (non-web) SceneBuilder push...Layer() method code style seems to be universally "everything on one line regardless of how long the line is", which doesn't look very maintainable so that file should probably be consolidated with whatever format is deemed appropriate here for the web versions of that class.

[yjbanov]

The change LGTM. The new code does read nicer. Did you just add trailing commas and run dartfmt on the code? We currently use dartfmt in the Web implementation.

[flar]

The change LGTM. The new code does read nicer. Did you just add trailing commas and run dartfmt on the code? We currently use dartfmt in the Web implementation.

Thoughts on the 2 different ways to format pushShaderMask()?

Actually, I just took the suggestion from @Hixie in this PR. If I run dartfmt on the new version of the files, though, it wants to move the fixed arguments into separate lines as well. If the web team is in the habit of running dartfmt on the dart source files then perhaps I should take its suggestion. I'm attaching diffs suggested by dartfmt.

compositing.dart diff
scene_builder.dart diff

[flar]

To be clear, this is what the Dart plugin in Android Studio wants to do with pushOffsetLayer if I try to move the fixed arguments to their own lines as dartfmt suggests:

  OffsetEngineLayer pushOffset(
      double dx,
      double dy, {
        OffsetEngineLayer oldLayer,
      });

I can reduce the indentation to 2 spaces by changing the continuation indent, but then continuations are wrong. I don't see any way to get it to not do an extra indent on the named parameters, though.

[yjbanov]

Thoughts on the 2 different ways to format pushShaderMask()?

I'd like to abstain :)

If the web team is in the habit of running dartfmt on the dart source files then perhaps I should take its suggestion.

Yes, let's apply dartfmt. If you don't use it now, someone else will eventually anyway.

[flar]

I added a few more trailing commas and ran dartfmt on all files that implement a SceneBuilder class.

I've now also adjusted the main version used for the native engine so I should get an approval from @liyuqian or @cbracken as well as @yjbanov

[yjbanov]

LGTM for the lib/web_ui parts

[liyuqian]

LGTM

[flar]

It seems that the analyzer likes things like:

    if (isComplexHint)
      hints |= 1;

but dartfmt decides it should be a single line, as in:

    if (isComplexHint) hints |= 1;

which the analyzer then complains about because it always wants conditional bodies on separate lines. Rather than making it even more verbose by adding {} braces, I decided to use ?: to initialize the hints in a single line.

    final int hints = (isComplexHint ? 1 : 0) | ...;

Thoughts @liyuqian?

[flar]

@liyuqian this was, debatably, the best compromise I could find to make both dartfmt and the analyzer happy without expanding this code section even more with {} braces and such. Details in the PR comments.

[liyuqian]

LGTM

[liyuqian]

It seems that the analyzer likes things like:

    if (isComplexHint)
      hints |= 1;

but dartfmt decides it should be a single line, as in:

    if (isComplexHint) hints |= 1;

which the analyzer then complains about because it always wants conditional bodies on separate lines. Rather than making it even more verbose by adding {} braces, I decided to use ?: to initialize the hints in a single line.

    final int hints = (isComplexHint ? 1 : 0) | ...;

Thoughts @liyuqian?

I like your new approach. In general, feel free to not strictly follow dartfmt if it conflicts with analyzer. I would also sometimes just reformat manually if I think dartfmt is not doing the right thing.

[yjbanov]

I believe over in the flutter/flutter side we're recommending to always use { }. This way both dartfmt and hand-written style do the same thing.

[liyuqian]

I believe over in the flutter/flutter side we're recommending to always use { }. This way both dartfmt and hand-written style do the same thing.

There's a similar debate in C++ code too. I believe many prefer

if (x) {
 y;
}

over just if (x) y;.

But some prefer if (x) y;. I'm in the former case because Skia enforces the more verbose format, and I get used to it.

[flar]

I like the use of {} in all cases because that minimizes diffs later if a second statement needs to be added to the body.

I decided to go with a style that was compatible with dartfmt in case, as @yjbanov said earlier, someone later decides to reformat the file.

I only like if (x) y; if the "y" is very simple, like a return. I was OK with the use here to set some hints, but not if it meant {}. Also, ?: works pretty well here.