[hartwork]

Fixes #161

Forward-port of 0.4.x pull requests #210 and #249

@kaixiong turns out that with enough mouse action over a longer period, I could still reproduce the stutter. That's why it's combined with dropping stale events now for a more complete fix, needs a backport to 0.4.x.

[kaixiong]

@hartwork, thanks this works nicely. The PR just needs some formatting fixes and it's good for merging.

[hartwork]

@hartwork, thanks this works nicely. The PR just needs some formatting fixes and it's good for merging.

@kaixiong what would you like me to change?

[kaixiong]

@hartwork This should be about all the formatting changes that should be applied now.

There are broader inconsistencies unrelated to this PR, but we'll have to save those edits for when we deploy Editorconfig project wide.

[kaixiong]

Use uniform (brace) initialization and have each member variable on its own line:

ResizeRequest()
    : m_width {0}
    , m_height {0}
{}

[kaixiong]

Opening brace for function bodies should be in a new line.

[kaixiong]

Opening brace for function bodies should be in a new line.

[kaixiong]

Private members should be placed at the bottom, below all public members.

[hartwork]

@hartwork This should be about all the formatting changes that should be applied now.

There are broader inconsistencies unrelated to this PR, but we'll have to save those edits for when we deploy Editorconfig project wide.

@kaixiong I can make those adjustments but let me say that with my style caring hat on I would disagree on some of these choices (..) and with my style caring hat off I would not make any changes like that outside of indentation fixes before we have clang-format (not just editorconfig) everywhere. I'll make these changes now, but I'll do them for you, not because I think that's what we should do

[kaixiong]

@kaixiong I can make those adjustments but let me say that with my style caring hat on I would disagree on some of these choices (..) and with my style caring hat off I would not make any changes like that outside of indentation fixes before we have clang-format (not just editorconfig) everywhere. I'll make these changes now, but I'll do them for you, not because I think that's what we should do

@hartwork, we could argue for days about style. But this is the convention we're sticking to for C++ code.

[hartwork]

@hartwork, we could argue for days about style.

@kaixiong we don't have to agree on style or talk about it for days, but with the current inconsistencies and the plans to add a tool, my understanding for these kind of changes is limited, because we'll reformat soon anyways. Let me get us started on a pull request introducing clang-format 16 shortly then, tuning of it's config would be up for discussion but for anything that the tool turns out it cannot be configured to do closer to the desired style — e.g. because of bugs or lack of a specific option — we'd have to live with that. Is that in your best interest too?

[hartwork]

@kaixiong PS: Changes applied by now

[kaixiong]

@kaixiong we don't have to agree on style or talk about it for days, but with the current inconsistencies and the plans to add a tool, my understanding for these kind of changes is limited, because we'll reformat soon anyways. Let me get us started on a pull request introducing clang-format 16 shortly then, tuning of it's config would be up for discussion but for anything that the tool turns out it cannot be configured to do closer to the desired style — e.g. because of bugs or lack of a specific option — we'd have to live with that. Is that in your best interest too?

@hartwork, out of the changes I requested, only the opening brace position and alignment of the constructor initialization can be reformatted by a tool.

The rest of the changes are choices picked up from other C++ developers and are used without fail (at least not intentionally) throughout Core.

• Private after public, because most of the time we just want to see the public interface in the class definition and not the internals.
• get_property() for getters so is to avoid name clashes with variables. I would prefer property() if this were not the case.
• Braces for constructor initialization lists because uniform initialization should be preferred (except when std::initalizer_listconstructors are involved).

It's precisely the project-wide reformatting that I'm dreading as the result looks nice but hurts blames and diffs. We've gone through it once in the past to replace tabs with spaces. I rather we have something close to the rest of the code now so the automated reformatting has less to change.

I have my pet peeves with the current formatting as well, like the space between function names and parameter lists. It's something I stuck with from the old C code and I would prefer to not change it for the reason above. There are other formatting choices that I'm still unsure of (plenty of pros and cons to weigh) but I'll save it for when we have the reformatting.

[hartwork]

@hartwork, out of the changes I requested, only the opening brace position and alignment of the constructor initialization can be reformatted by a tool.

@kaixiong that's understood, yes.

The rest of the changes are choices picked up from other C++ developers and are used without fail (at least not intentionally) throughout Core.

It didn't feel like that to me, but okay.

• Private after public, because most of the time we just want to see the public interface in the class definition and not the internals.

That's one way to see it. Not a fan of it, but I can accept it.

• get_property() for getters so is to avoid name clashes with variables. I would prefer property() if this were not the case.

The m_ prefix resolves that problem. E.g. Qt goes against get_ on purpose, both sides have pros and cons.

• Braces for constructor initialization lists because uniform initialization should be preferred (except when std::initalizer_listconstructors are involved).

That's beyond formatting and btw I fully support it 👍

It's precisely the project-wide reformatting that I'm dreading as the result looks nice but hurts blames and diffs. We've gone through it once in the past to replace tabs with spaces. I rather we have something close to the rest of the code now so the automated reformatting has less to change.

For the blame part, there are two things that can reduce the pain: Using a commit author like clang-format <clang-format@tools.invalid> or using something like git-hyper-blame rather than git blame. Personally, I'm already happy with former. Mass-changes like that sure have downsides and need to be timed well but auto-formatting kills all discussions about formatting in pull requests for good, that's invaluable in my book. Things get better after it's done.

I have my pet peeves with the current formatting as well, like the space between function names and parameter lists. It's something I stuck with from the old C code and I would prefer to not change it for the reason above. There are other formatting choices that I'm still unsure of (plenty of pros and cons to weigh) but I'll save it for when we have the reformatting.

My theory is that for some people it's primarily about beauty, for some primarily about avoiding pain, for other primarily about friction. If we can find some formatting common ground where your and my pain is at a tolerable level, I would call that a success.

Are there more changes that you would like me to do for this pull request?

[kaixiong]

@hartwork

Thank you for the changes.

Let me address the main issue first before I go on:

Are there more changes that you would like me to do for this pull request?

One last one: use get_ for getters. This is the convention in the rest of Core and lv-tool.

• Private after public, because most of the time we just want to see the public interface in the class definition and not the internals.

That's one way to see it. Not a fan of it, but I can accept it.

Public first is a very common approach in C++ where headers are used as a kind of documentation. When designing classes, it also makes practical sense to consider and define the public interface first, then elucidate the implementation details.

This wasn't always how I've coded in C++. I adopted the public-first ordering after reading others' code and never looked back. This approach is also enshrined in various C++ style guides (e.g. Google and Qt). Public, protected, then private. Some have additional recommendations or rules that order members by type e.g. typedefs, enums, constructors, destructor, etc. I do something similar in Core (and in my own code), but nothing too strict.

The m_ prefix resolves that problem. E.g. Qt goes against get_ on purpose, both sides have pros and cons.

True. I instinctively adopted the get_ and set_ convention from gtkmm for two reasons:

1. I've never been really clear on how the C/C++ compiler decides when there's a name resolution conflict between a local variable name, parameter name, and a function name.
2. get_ and set_ follow the view that all class method names are actions.

For the blame part, there are two things that can reduce the pain: Using a commit author like clang-format <clang-format@tools.invalid> or using something like git-hyper-blame rather than git blame. Personally, I'm already happy with former. Mass-changes like that sure have downsides and need to be timed well but auto-formatting kills all discussions about formatting in pull requests for good, that's invaluable in my book. Things get better after it's done.

True but up to a point. Some formatting cannot be distilled and encoded into machine rules because they're aesthetic, like aligning assignment operators in a column. They can look good or bad (neatness vs over-spacing) depending on actual code.

As far as Core is concerned, the styling has been relatively consistent (with the exception of uniform initialization) since I reformatted most of it during the C++ conversion. I do not find any massive reformatting in Core justified.

lv-tool is a bit more mixed as Daniel (rigid) wrote a lot of the code and the formatting wasn't enforced then. Its conventions are close to Core and should simply follow Core.

The plugins are the messy ones and I've opted not to change it so far.

These considerations are mentioned in the hacking guide in case you haven't seen it. Its bottom line is, follow the existing style (unless it's so inconsistent you can't discern it). Consistency is the priority. I follow this principle when working with any codebase, whether it's mine or someone's else.

I'm for a formatting tool not for killing debates. Rather, it's to correct the odd mistakes like forgetting a space, mixed tabs/spaces, etc. To that end, I'm more keen on applying Editorconfig rather than clang-format.

My theory is that for some people it's primarily about beauty, for some primarily about avoiding pain, for other primarily about friction. If we can find some formatting common ground where your and my pain is at a tolerable level, I would call that a success.

To me it's about both. I have a perfectionist streak and strive to write beautiful and clean code. Some people have even said I'm too obsessed with it (observe how often I edit my comments for minor formatting corrections). But as with all things in life, we have to weigh 'beauty' against other considerations.

The one thing I've learned from reading lots of FOSS project code (some large, some small) is that - even the dreaded GNU formatted ones with the odd indentation of braces - is that consistency is beauty.

[hartwork]

One last one: use get_ for getters. This is the convention in the rest of Core and lv-tool.

@kaixiong done, pushed.

This is not an easy reply to write e.g. because there is a risk of this becoming a flavor of vim-versus-emacs, just on another topic, and because of the damage that these topics can do. I'm trying to balance authenticity and damage control here.

Regarding whether "massive reformatting in Core [is] justified": Massive depends on tuning of the tool and whether reducing amount is worth the time. The key is use of a tool. I believe for formatting tools, humans should control the formatter's config file — to avoid consistently bad — and the machine is then made the authority on code formatting based on that config file. Whenever the machine doesn't do the right thing, the config file is adjusted and all files are affected (and made consistent!). It's not about ultimate beauty to me but about avoiding severe unbeauty, unreadability, unnecessary distractions, both while reading ready code and while discussing pull requests — formatting distracts from the actual discussion of value. It is shades of gray, no gold. With a formatter you decide things for all files at ones — multiplication —, not single cases here and there again and again adding up — addition. I am happy to fix semantics, naming, security and so on but I want to have near-zero formatting discussions in my daily life midterm as a non-extrovert in any project, because of its poor value and it's very real cost. Please help us find some way to get there, life is too short to not outsource whitespace adjustments to machines. If clang-format's result are not pretty enough, let's use another formatter or fix the config. Let's add editorconfig, sure, but having editorconfig will only make a difference code reaches CI — once we have a formatter, CI is the real deal, and editorconfig is a free byproduct, not with local editing, but regarding the eventual code.

[kaixiong]

One last one: use get_ for getters. This is the convention in the rest of Core and lv-tool.

@kaixiong done, pushed.

@hartwork, thank you!

This is not an easy reply to write e.g. because there is a risk of this becoming a flavor of vim-versus-emacs, just on another topic, and because of the damage that these topics can do. I'm trying to balance authenticity and damage control here.

I agree. I dread threads like these as well but feel I need to make my views clear.

Regarding whether "massive reformatting in Core [is] justified": Massive depends on tuning of the tool and whether reducing amount is worth the time. The key is use of a tool. I believe for formatting tools, humans should control the formatter's config file — to avoid consistently bad — and the machine is then made the authority on code formatting based on that config file. Whenever the machine doesn't do the right thing, the config file is adjusted and all files are affected (and made consistent!). It's not about ultimate beauty to me but about avoiding severe unbeauty, unreadability, unnecessary distractions, both while reading ready code and while discussing pull requests — formatting distracts from the actual discussion of value. It is shades of gray, no gold. With a formatter you decide things for all files at ones — multiplication —, not single cases here and there again and again adding up — addition. I am happy to fix semantics, naming, security and so on but I want to have near-zero formatting discussions in my daily life midterm as a non-extrovert in any project, because of its poor value and it's very real cost. Please help us find some way to get there, life is too short to not outsource whitespace adjustments to machines. If clang-format's result are not pretty enough, let's use another formatter or fix the config. Let's add editorconfig, sure, but having editorconfig will only make a difference code reaches CI — once we have a formatter, CI is the real deal, and editorconfig is a free byproduct, not with local editing, but regarding the eventual code.

We are in full agreement regarding formatting. There is value in consistency and I prize it as much as you do. My pragmatic take is this: let's reformat to fix those inconsistent spacing, but only do it for Core and friends (i.e. excluding plugins). I expect that Core being fairly consistent should see relatively few changes if we tweak the formatter(s) just right. The reformatting will have the biggest impact on lv-tool which I feel is unavoidable.

Let's move the discussion over clangd-format PR.