[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
From:       Ivan =?utf-8?B?xIx1a2nEhw==?= <ivan.cukic () kde ! org>
Date:       2020-08-16 15:00:23
Message-ID: 5834853.Mh6RI2rZIc () drako
[Download RAW message or body]

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@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12


[prev in list] [next in list] [prev in thread] [next in thread]