Add clang format and editorconfig files.

[tarek-y-ismail]

This is a PR to discuss if the current state of clang-format fits our desired needs (beautifully formatted code).

Based on the clang format config proposed by @hbatagelo. Decided to sneak in an editorconfig file since that also helped.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.45%. Comparing base (317060e) to head (5edd780).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3455      +/-   ##
==========================================
- Coverage   77.45%   77.45%   -0.01%     
==========================================
  Files        1073     1073              
  Lines       68782    68782              
==========================================
- Hits        53278    53274       -4     
- Misses      15504    15508       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RAOF]

Thanks for trying this. I think the changing of "Construtor() : constructee-list" to be on a different line should probably be fixed to our current style of

Type::Type()
    : stuff(),
      bits()
{

as the current formatting config here ends up doing this:

MirPointerEvent::MirPointerEvent(
    MirInputDeviceId dev,
    std::chrono::nanoseconds et,
    MirInputEventModifiers mods,
    MirPointerAction action,
    MirPointerButtons buttons,
    std::optional<geom::PointF> position,
    geom::DisplacementF motion,
    MirPointerAxisSource axis_source,
    mev::ScrollAxisH h_scroll,
    mev::ScrollAxisV v_scroll) :
    MirInputEvent(mir_input_event_type_pointer, dev, et, mods),
    position_{position},
    motion_{motion},
    axis_source_{axis_source},
    h_scroll_{h_scroll},
    v_scroll_{v_scroll},
    action_{action},
    buttons_{buttons}

which I think is a significant downgrade in readability.

[RAOF]

(Haven't finished reviewing the proposed formatting changes. That's just the big thing I've disliked so far)

[AlanGriffiths]

Thanks for trying this. I think the changing of "Construtor() : constructee-list" to be on a different line should probably be fixed to our current style of

We have three "acceptable formats", one of which is the one you don't like:

https://canonical-mir.readthedocs-hosted.com/stable/_static/cppguide/#Constructor_Initializer_Lists

[mattkae]

My nits:

1. I prefer:

   spec.exclusive_rect = exclusive_rect ?
      mir::optional_value<geom::Rectangle>(exclusive_rect.value()) :
      mir::optional_value<geom::Rectangle>();

over

   spec.exclusive_rect = exclusive_rect ? mir::optional_value<geom::Rectangle>(exclusive_rect.value()) :
                                         mir::optional_value<geom::Rectangle>();

2. I prefer:

  return persistent_surface_store([]()
  {
      return std::make_shared<msh::DefaultPersistentSurfaceStore>();
  });

over

  return persistent_surface_store(
      []()
      {
          return std::make_shared<msh::DefaultPersistentSurfaceStore>();
      });

Other than that, I more-or-less like this a lot!

[AlanGriffiths]

3. I prefer:

  return persistent_surface_store([]()
  {
      return std::make_shared<msh::DefaultPersistentSurfaceStore>();
  });

over

  return persistent_surface_store(
      []()
      {
          return std::make_shared<msh::DefaultPersistentSurfaceStore>();
      });

Other than that, I more-or-less like this a lot!

I've been using

  return persistent_surface_store([]()
      {
          return std::make_shared<msh::DefaultPersistentSurfaceStore>();
      });

[KnockerPulsar]

Thanks for trying this. I think the changing of "Construtor() : constructee-list" to be on a different line should probably be fixed to our current style of

Type::Type()
    : stuff(),
      bits()
{

as the current formatting config here ends up doing this:

MirPointerEvent::MirPointerEvent(
    MirInputDeviceId dev,
    std::chrono::nanoseconds et,
    MirInputEventModifiers mods,
    MirPointerAction action,
    MirPointerButtons buttons,
    std::optional<geom::PointF> position,
    geom::DisplacementF motion,
    MirPointerAxisSource axis_source,
    mev::ScrollAxisH h_scroll,
    mev::ScrollAxisV v_scroll) :
    MirInputEvent(mir_input_event_type_pointer, dev, et, mods),
    position_{position},
    motion_{motion},
    axis_source_{axis_source},
    h_scroll_{h_scroll},
    v_scroll_{v_scroll},
    action_{action},
    buttons_{buttons}

which I think is a significant downgrade in readability.

Regarding this, the closest I could get was using the following settings:

-AlignAfterOpenBracket: AlwaysBreak
+AlignAfterOpenBracket: Align
-BreakConstructorInitializers: AfterColon
+BreakConstructorInitializers: BeforeColon

Which changes the example you gave to:

 MirPointerEvent::MirPointerEvent(MirInputDeviceId dev,
-                    std::chrono::nanoseconds et,
-                    MirInputEventModifiers mods,
-                    MirPointerAction action,
-                    MirPointerButtons buttons,
-                    std::optional<geom::PointF> position,
-                    geom::DisplacementF motion,
-                    MirPointerAxisSource axis_source,
-                    mev::ScrollAxisH h_scroll,
-                    mev::ScrollAxisV v_scroll)
+                                 std::chrono::nanoseconds et,
+                                 MirInputEventModifiers mods,
+                                 MirPointerAction action,
+                                 MirPointerButtons buttons,
+                                 std::optional<geom::PointF> position,
+                                 geom::DisplacementF motion,
+                                 MirPointerAxisSource axis_source,
+                                 mev::ScrollAxisH h_scroll,
+                                 mev::ScrollAxisV v_scroll)
     : MirInputEvent(mir_input_event_type_pointer, dev, et, mods),
       position_{position},
       motion_{motion},

I'm not sure how it changes other parts of the code so I'm interested in everyone's opinion.

[AlanGriffiths]

 MirPointerEvent::MirPointerEvent(MirInputDeviceId dev,
-                    std::chrono::nanoseconds et,
-                    MirInputEventModifiers mods,
-                    MirPointerAction action,
-                    MirPointerButtons buttons,
-                    std::optional<geom::PointF> position,
-                    geom::DisplacementF motion,
-                    MirPointerAxisSource axis_source,
-                    mev::ScrollAxisH h_scroll,
-                    mev::ScrollAxisV v_scroll)
+                                 std::chrono::nanoseconds et,
+                                 MirInputEventModifiers mods,
+                                 MirPointerAction action,
+                                 MirPointerButtons buttons,
+                                 std::optional<geom::PointF> position,
+                                 geom::DisplacementF motion,
+                                 MirPointerAxisSource axis_source,
+                                 mev::ScrollAxisH h_scroll,
+                                 mev::ScrollAxisV v_scroll)

This style I'm against: generally the amount of whitespace at the start of lines shouldn't depend on the text on the preceding line. That can create a lot of noise in diffs (or inconsistent formatting) if the text changes.

See https://canonical-mir.readthedocs-hosted.com/stable/_static/cppguide/#Constructor_Initializer_Lists for the styles we previously agreed on

[KnockerPulsar]

@mattkae
For point 1, it seems that breaking here is dependent on the max line width. 80 seems to do something close, but I think it'll break lots of other stuff. I tried playing with other flags to no avail.

For point 2, I also tried playing with a few flags, but I think this is not achievable as well without breaking other things

[KnockerPulsar]

This style I'm against: generally the amount of whitespace at the start of lines shouldn't depend on the text on the preceding line. That can create a lot of noise in diffs (or inconsistent formatting) if the text changes.

I agree with you on this. I think AlignAfterOpenBracket: DontAlign standardizes the indent to the third style mentioned in the style guide for all constructors. But I don't think it's as readable as the existing style guide in the example Chris gave.

[AlanGriffiths]

the existing style in the example Chris gave

By "the existing style" what do you mean? One of the three styles we use now? Which? Or the proposed style?

[tarek-y-ismail]

By "the existing style" what do you mean? One of the three styles we use now? Which? Or the proposed style?

Seems that I forgot to type "guide". I meant the style guide you linked before. Sorry!

Also it seems that my browser decided to troll me by signing into my personal github account, it's too late to delete and repost my previous comments with this account so I'll just start commenting with it starting now :)