Qt Coding Style + clang-format + kate.git

[Christoph Cullmann]

On 2019-08-24 21:02, Dominik Haumann wrote:
> Btw, we can discuss at Akademy again...
Yes, we can take there a look, thought not that many people are there
that are concerned :=)

For the issues:

I think most of them stem from the too short column limit,
like I mentioned before, I think that should be raised to some 
reasonable amount.

That should avoid any strange re-flowing of comments, if not, we could 
turn that off
completely.

The initializer stuff can be tuned to use , m_..., we do that at
company, too.

For the const ll &x, yes, that is the Qt style and I would prefer it 
that way.

Perhaps these concerns should be raised in

https://phabricator.kde.org/T11214

too, as I would prefer that we use some "common" style between the KDE 
sub-projects.

Greetings
Christoph

> 
> On Sat, Aug 24, 2019 at 8:53 PM Dominik Haumann <dhaumann at kde.org>
> wrote:
> 
>> Hi,
>> 
>> I just applied clang-format to Kate's source code. And already
>> looking at kateapp.cpp we find:
>> 
>> KateApp::KateApp(const QCommandLineParser &args)
>> -    : m_args(args)
>> -    , m_wrapper(appSelf = this)
>> -    , m_docManager(this)
>> -    , m_adaptor(this)
>> -    , m_pluginManager(this)
>> -    , m_sessionManager(this)
>> +    : m_args(args),
>> +      m_wrapper(appSelf = this),
>> +      m_docManager(this),
>> +      m_adaptor(this),
>> +      m_pluginManager(this),
>> +      m_sessionManager(this)
>> 
>> Is there an option for clang-format to prefer the old behavior? The
>> leading comma allows us to have much cleaner diffs with better
>> history.
>> 
>> Let's continue:
>> 
>> -    qputenv("KATE_PID",
>> 
> QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());
>> +    qputenv("KATE_PID",
>> +
>> 
> QStringLiteral("%1").arg(QCoreApplication::applicationPid()).toLatin1().constData());
>> 
>> Ok, although given wide-screens I think the old variant is more
>> readable. It gets more apparent here:
>> 
>> -    } else if (!m_args.isSet(QStringLiteral("stdin")) &&
>> (m_args.positionalArguments().count() == 0)) { // only start session
>> if no files specified
>> +    } else if (!m_args.isSet(QStringLiteral("stdin"))
>> +               && (m_args.positionalArguments().count()
>> +                   == 0)) { // only start session if no files
>> specified
>> 
>> I really cannot say that the new version looks better to me.
>> 
>> Continuing:
>> 
>> KMessageBox::sorry(activeKateMainWindow(),
>> -                               i18n("The file '%1' could not be
>> opened: it is not a normal file, it is a folder.",
>> info.url.toString()));
>> +                               i18n("The file '%1' could not be
>> opened: it is not a normal file, "
>> +                                    "it is a folder.",
>> +                                    info.url.toString()));
>> 
>> Not convinced.
>> 
>> -    for (const auto& window : m_mainWindows) {
>> +    for (const auto &window : m_mainWindows) {
>> 
>> Now, do we want the & left or right-aligned?
>> 
>> Let's have a look into the header file:
>> 
>> /**
>> -     * Close the given \p document. If the document is modified,
>> user will be asked if he wants that.
>> -     * \param document the document to be closed
>> -     * \return \e true on success, otherwise \e false
>> +     * Close the given \p document. If the document is modified,
>> user will be asked if he wants
>> +     * that. \param document the document to be closed \return \e
>> true on success, otherwise \e
>> +     * false
>> */
>> 
>> The reformatted version breaks the doxygen comment. Ok, maybe
>> doxygen still gets it right, but the \return in the same line is
>> imho totally wrong.
>> 
>> Unfortunately with current config: -2 (rejected) from my side. Maybe
>> there is a better config, but breaking the comments this way is not
>> worth it.
>> 
>> Best regards
>> Dominik
>> 
>> On Sat, Aug 24, 2019 at 4:31 PM Christoph Cullmann
>> <christoph at cullmann.io> wrote:
>> 
>>> On 2019-08-24 07:48, Dominik Haumann wrote:
>>>> No, will do later today.
>>> 
>>> Thanks!
>>> 
>>> I did inspect my local result with the config again.
>>> 
>>> The only thing I might want to change is the
>>> 
>>> # Column width is limited to 100 in accordance with Qt Coding
>>> Style.
>>> # https://wiki.qt.io/Qt_Coding_Style
>>> # Note that this may be changed at some point in the future.
>>> ColumnLimit: 100
>>> 
>>> part.
>>> 
>>> For stuff with long variable + function names, you get ugly wraps.
>>> 
>>> In company we settled to some 240 limit.
>>> No limit doesn't work well either, as otherwise a lot of stuff
>>> will
>>> just be collapsed to "LONG" lines.
>>> 
>>> Greetings
>>> Christoph
>>> 
>>> --
>>> Ignorance is bliss...
>>> https://cullmann.io | https://kate-editor.org

-- 
Ignorance is bliss...
https://cullmann.io | https://kate-editor.org