Qt Coding Style + clang-format + kate.git

[Dominik Haumann]

Btw, we can discuss at Akademy again...

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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20190824/7e443ca4/attachment.html>