add markdown support
ClosedPublic

Authored by progwolff on Aug 18 2017, 10:20 AM.

Details

Reviewers
aacid
Group Reviewers
Okular
Commits
R223:4931385777ce: add markdown support
Summary

Adds support for Markdown documents

BUG: 360603

Test Plan

Open a Markdown (.md) document in Okular

Diff Detail

Repository
R223 Okular
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
progwolff created this revision.Aug 18 2017, 10:20 AM
Restricted Application added a project: Okular. · View Herald TranscriptAug 18 2017, 10:20 AM
progwolff retitled this revision from add markdown support BUG: 360603 to add markdown support.Aug 18 2017, 10:21 AM
progwolff edited the summary of this revision. (Show Details)
aacid added a subscriber: aacid.Aug 18 2017, 8:59 PM

Interesting work :)

Some inline comments

cmake/modules/FindDiscount.cmake
23

So the library is called discount but installs itself as libmarkdown?

Doesn't it have a pkg-config file?

generators/markdown/converter.cpp
42

You probably want toLocal8Bit here

57

Have you tried mkd_document so we don't need a temp file?

generators/markdown/hi32-app-okular-md.svg
1 ↗(On Diff #18328)

Did you draw this SVG?

generators/markdown/okularMd.desktop
11

You need to drop all translations from these and the rest of the files.

progwolff added inline comments.Aug 18 2017, 10:35 PM
cmake/modules/FindDiscount.cmake
23

At least the Arch Linux package (which is called "discount") does not install a pkg-config.

The naming seems a little inconsistent. The library, i.e. the specific implementation of a markdown converter is called "discount". The installed .so is called libmarkdown, which better describes the function it provides.

generators/markdown/converter.cpp
57

I played around with this function, but could not get it to work. I'll take a deeper look into the source files of discount tomorrow, to check if we can get rid of the temporary file.

generators/markdown/hi32-app-okular-md.svg
1 ↗(On Diff #18328)

It's taken from here: https://github.com/dcurtis/markdown-mark
Licensed under Creative Commons Zero.

progwolff updated this revision to Diff 18379.Aug 19 2017, 9:13 AM
  • Simplify markdown conversion
  • Drop translations
  • Fix file name encoding
aacid added a comment.Aug 22 2017, 9:14 PM

It doesn't build on Ubuntu 17.04

/home/kdeunstable/okular/generators/markdown/converter.cpp: In member function ‘virtual QTextDocument* Markdown::Converter::convert(const QString&)’:
/home/kdeunstable/okular/generators/markdown/converter.cpp:46:40: error: ‘MKD_FENCEDCODE’ was not declared in this scope
   if ( !mkd_compile( markdownHandle, MKD_FENCEDCODE | MKD_GITHUBTAGS | MKD_AUTOLINK ) ) {
                                      ^~~~~~~~~~~~~~
/home/kdeunstable/okular/generators/markdown/converter.cpp:46:57: error: ‘MKD_GITHUBTAGS’ was not declared in this scope
   if ( !mkd_compile( markdownHandle, MKD_FENCEDCODE | MKD_GITHUBTAGS | MKD_AUTOLINK ) ) {

Uses discount 2.1.8-2

generators/markdown/converter.cpp
58

980x1307 is an interesting size, any specific reaosn you choose that one?

progwolff updated this revision to Diff 18577.Aug 23 2017, 9:02 AM
  • fix build with discount < 2.2.2
In D7382#138593, @aacid wrote:

It doesn't build on Ubuntu 17.04

In this revision unknown flags are defined to 0. This should allow us to build with older versions of discount.
I also set the minimum version to 2.

I don't have access to a Ubuntu system right now. Could you try it again?

generators/markdown/converter.cpp
58

980 is the width github uses to show README.md documents. This means many documents will be optimized for this width.
980x1307 then matches the aspect ratio of 600x800, wich is used by other generators.

progwolff updated this revision to Diff 18578.Aug 23 2017, 9:03 AM
  • fix typo

Builds and runs now :)

generators/markdown/converter.cpp
118

Can you double check this -1?

If i enable the draw borders around links in the accessibility settings i see the link ends one character earlier than it should, no idea if this is this -1 but i did a random guess :D

generators/markdown/generator_md.cpp
31

i would just use text-markdown here instead okular-md, breeze provides it and this way we don't need to ship icons

generators/markdown/libokularGenerator_md.json
26

this should be true since you actually provide settings

progwolff updated this revision to Diff 18653.Aug 24 2017, 7:32 AM
  • fix browse action width
  • use system provided icon
  • enable internal settings
progwolff marked an inline comment as done.Thu, Aug 24, 7:35 AM

Okay, now using system provided icons.
The width of the browse actions is also fixed.
X-KDE-okularHasInternalSettings is set to true.

Thanks for your help and patience!

generators/markdown/converter.cpp
118

You're right, fixed this.

aacid accepted this revision.Thu, Aug 24, 11:04 PM

Looks, good thanks, i'll commit it :)

There's a few minor issues, and a structural problem i just found, but i won't bother you with those, unless you want to do more stuff, if so ping me :)

This revision is now accepted and ready to land.Thu, Aug 24, 11:04 PM
aacid closed this revision.Thu, Aug 24, 11:05 PM