Hi Ivan,
Am Sonntag, 16. August 2020, 17:00:23 CEST schrieb Ivan Čukić:
> Hi Friedrich,
>
> Very nice, thanks for doing this!
Glad to see people liking it, so my time was more worth it :)
And thank you for review.
> These are the few nit-picks (feel free to ignore them) I have:
Nitpicks are welcome with me :) Actually taking culprit in not having run any
code checker before uploading, bad me. And trying to excuse that with "Was
just a quick proof of feasibility for others to use" sounds awful typing it,
darn, no way out, lesson learned.
Hm, I could blame the wiki page
https://community.kde.org/ReleasingSoftware#Sanity_Checklist
not yet having mentioned running sanity tools :)
Added that there for everyone's next time now.
> markdownview.cpp:55
>
> The `hasSelection` variable makes the code look much more complex than it
> is. It can be removed and the last argument of the `Q_EMIT
> contextMenuRequested` call set to:
> !linkUrl.isValid() && hasSelection()
Yes, does look strange without context, will clean up. Though still as
separate variable, so the parameter as passed in the emit call gets some
semantic by the variable name.
Motivation was to keep the code similar to patterns used in KWebKitPart &
WebEnginePart, to allow some more detailed context menu once possible. Current
QTextDocument/QTextBrowser API though does not provide same level of detail,
so resulting in that empty logic, which I can see how it confuses.
Adding a comment instead, so the next person extending things knows what to
look at to keep things consistent.
> markdownpartfactory.cpp:45
>
> Personal preference - use `auto` when you have `= new Something(:::)` on the
> right - no need for `Something` to be written twice:
> Something* p = new Something(...)
>
> The variable part can even go away - just return new MarkdownPart.
>
> Similar lines exist in markdownpart.cpp, though there you use auto almost in
> all these cases.
Not repeating type name is also my preference (though with pointer indicator *
-> "auto*" here to make code more obvious). Excuse: this was the old code just
copied from kmarkdownwebview I did not have to touch, so stayed away from
messing with. Now you made me (will backport also to keep diff small).
Also removed temporary variable as proposed, development left-over.
> markdownbrowserextension.cpp:51
>
> There is no point in having the `const bool forcesNewWindow = false`
> declared at the beginning of the function when it is used only in a single
> line at the end of the function:
> bargs.setForcesNewWindow(forcesNewWindow);
>
> Replacing this with false is IMO more readable as you don't need to scroll
> up to check what the value of forcesNewWindow is:
> bargs.setForcesNewWindow(false);
Same reasoning as with hasSelection of the context menu. And will do same
resolution, making code simple, but pointing to similar code for consistency
in handling on potential future changes.
Should be all dealt with now in latest master :)
Cheers
Friedrich
