D14738: Add the markdown entry

Pino Toscano noreply at phabricator.kde.org
Tue Aug 14 17:52:52 BST 2018


pino added a comment.


  Nitpick: please provide a consistent indentation in the new files -- they seem to mix spaces and tabs.
  Also, was this tested when cantor is built without the Discount library? How does it behave when trying to add/edit markdown entries, and trying to load worksheets with such entries?

INLINE COMMENTS

> CMakeLists.txt:103
> +                     KF5::KIOCore KF5::KIOFileWidgets KF5::KIOWidgets
> +                     Qt5::PrintSupport cantorlibs cantor_config)
>  if(LIBSPECTRE_FOUND)

Extra whitespace change.

> CMakeLists.txt:108
> +if(Discount_FOUND)
> +    target_link_libraries(cantorpart ${Discount_LIBRARIES})
> +endif(Discount_FOUND)

FindDiscount.cmake creates a `Discount::Lib` imported library that you can use, and it's even better to do so: it makes sure the target has both the right include directories and the right libraries needed.

> cantor_part.rc:2
>  <!DOCTYPE kpartgui SYSTEM "kpartgui.dtd">
>  <kpartgui name="cantor_part" version="2">
>  <MenuBar>

Since new actions are added here, you must bump the version number of the ui .rc file, otherwise users with a customized menubar/toolbar will not see any change.

> markdownentry.cpp:31
> +
> +#include <QDebug>
> +

This can be removed, once the extra debug is removed too.

> markdownentry.cpp:56
> +    m_textItem->setPlainText(plain);
> +    qDebug() << plain;
> +}

Extra debug, no need for it.

> markdownentry.cpp:63
> +
> +    qDebug() << plain;
> +    QDomElement el = doc.createElement(QLatin1String("Markdown"));

Ditto.

> markdownentry.cpp:78
> +    QByteArray mdCharArray = plain.toUtf8();
> +    MMIOT* mdHandle = mkd_string(mdCharArray.data(), mdCharArray.size()+1, 0); // get the size of the string in byte
> +    if(!mkd_compile(mdHandle, MKD_NOSUPERSCRIPT | MKD_FENCEDCODE | MKD_GITHUBTAGS))

This is leaked in all the cases (i.e. when `mkd_compile` succeedes or not). You need to call `mkd_cleanup()`.

REPOSITORY
  R55 Cantor

REVISION DETAIL
  https://phabricator.kde.org/D14738

To: kqwyf, pino, #cantor, filipesaraiva
Cc: sirgienko, filipesaraiva, pino, asemke, kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180814/779de7f8/attachment.html>


More information about the kde-edu mailing list