KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed

Ivan Čukić ivan.cukic at kde.org
Sun Aug 16 16:00:23 BST 2020


Hi Friedrich,

Very nice, thanks for doing this!

These are the few nit-picks (feel free to ignore them) I have:

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()


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.


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);


Cheers,
Ivan


-- 
dr Ivan Čukić
ivan at cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12






More information about the kde-core-devel mailing list