[DONE] Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Hi, Am Samstag, 15. August 2020, 19:20:53 CEST schrieb Friedrich W. H. Kossebau: > what is markdownpart you ask? A KParts plugin allowing to view markdown > documents rendered e.g. in Kate's preview plugin, Ark, Krusader or > Konqueror, being mainly a wrapper around QTextDocument::setMarkdown & > QTextBrowser. See here it being used with Kate's preview plugin: > https://cdn.kde.org/screenshots/markdownpart/markdownpart.png > Note (edit: updated): Qt 5.14 minimum needed > > I would like to propose markdownpart for a move into community maintenance > mode and becoming part of release service managed projects starting with RS > 20.12. It would match graphics/svgpart in the mode (whose code mode BTW is > aöso similar, mainly a wrapper around QSvgRenderer & QGraphicsView). Am Montag, 24. August 2020, 00:30:13 CEST schrieb Friedrich W. H. Kossebau: > Small plan change here: > given 20.12 is quite some weeks away still, markdownpart should move post- > kdereview first into extragear, for one or two releases with the initial > feedback added and especially translations, and only then enter release > service latest in November before the repo freeze. Thanks everyone for their review & comments. Without any open objections or todos, I am now executing the sketchecd plan: * move markdownpart into extragear/utils today * do official string freeze on trunk today * do a 0.1.1 release on Monday, September 21st from trunk (so translators have 3 weeks occasion to add support for their locale) * move into release service set before November (waiting a bit after 0.1.1 in chance bugs are reported which should be fixed in a release before 20.12) Cheers Friedrich
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Am Samstag, 15. August 2020, 19:20:53 CEST schrieb Friedrich W. H. Kossebau: > Note: for now Qt 5.15-only, 5.14 possible but untested. BTW, thanks to feedback the min dependencies are now lowered to Qt 5.14 & KF 5.66. > I would like to propose markdownpart for a move into community maintenance > mode and becoming part of release service managed projects starting with RS > 20.12. It would match graphics/svgpart in the mode (whose code mode BTW is > aöso similar, mainly a wrapper around QSvgRenderer & QGraphicsView). Small plan change here: given 20.12 is quite some weeks away still, markdownpart should move post- kdereview first into extragear, for one or two releases with the initial feedback added and especially translations, and only then enter release service latest in November before the repo freeze. Makes sense? Otherwise thanks for review & feedback so far. Seems there have no obstacles come up so far, so upcoming Saturday markdownpart can then leave the current stage, and would go to extragear/utils, right into preparation of first full release. Cheers Friedrich
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Am Freitag, 21. August 2020, 23:34:03 CEST schrieb Albert Astals Cid: > El dilluns, 17 d’agost de 2020, a les 23:04:44 CEST, Friedrich W. H. Kossebau va escriure: > > But not my call, after all I offer this to KDE community for adoption, sou > > your choice. > > I'm a bit concerned about this being "abandonware" from birth, but seems > there's relative interest from the community to collectively maintain it so > not going to complain if this ends up in release service. Thanks for statement, Albert. Cheers Friedrich
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
El dilluns, 17 d’agost de 2020, a les 23:04:44 CEST, Friedrich W. H. Kossebau va escriure: > But not my call, after all I offer this to KDE community for adoption, sou > your choice. I'm a bit concerned about this being "abandonware" from birth, but seems there's relative interest from the community to collectively maintain it so not going to complain if this ends up in release service. Cheers, Albert > > Cheers > Friedrich > > >
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Ivan Čukić wrote: > 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. IMHO, this just makes the code harder to read (as most uses of "auto"). Kevin Kofler
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
On 15/08/20 19:20, Friedrich W. H. Kossebau wrote: > Hi, > > what is markdownpart you ask? A KParts plugin allowing to view markdown > documents rendered e.g. in Kate's preview plugin, Ark, Krusader or Konqueror, > being mainly a wrapper around QTextDocument::setMarkdown & QTextBrowser. > See here it being used with Kate's preview plugin: > https://cdn.kde.org/screenshots/markdownpart/markdownpart.png > Note: for now Qt 5.15-only, 5.14 possible but untested. > > I would like to propose markdownpart for a move into community maintenance > mode and becoming part of release service Hi Friedrich, why not merge this plugin into kate instead? > > Cheers > Friedrich > > Cheers, Elvis
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
On 17/08/20 23:04, Friedrich W. H. Kossebau wrote: > Hi Elvis, > > Am Montag, 17. August 2020, 22:43:37 CEST schrieb Elvis Angelaccio: >> On 15/08/20 19:20, Friedrich W. H. Kossebau wrote: >>> Hi, >>> >>> what is markdownpart you ask? A KParts plugin allowing to view markdown >>> documents rendered e.g. in Kate's preview plugin, Ark, Krusader or >>> Konqueror, being mainly a wrapper around QTextDocument::setMarkdown & >>> QTextBrowser. See here it being used with Kate's preview plugin: >>> https://cdn.kde.org/screenshots/markdownpart/markdownpart.png >>> Note: for now Qt 5.15-only, 5.14 possible but untested. >>> >>> I would like to propose markdownpart for a move into community maintenance >>> mode and becoming part of release service >> >> Hi Friedrich, >> >> why not merge this plugin into kate instead? > > Let me answer with another question: > why with Kate and not with Ark or with Krusader or with Konqueror or any > other > potential KParts plugin user where Markdown viewing often is useful? > > Bundling with Kate would be a rather random choice IMHO. And > a) make it challenging for packagers to split out an independent packager for > the markdown kpart with all the files belonging to it > b) make it harder for anyone interested in hacking on it, as they would have > to also care about the whole Kate repo and its complete build system, when > they just want to improve the markdown viewer e.g. for Ark. > > But not my call, after all I offer this to KDE community for adoption, sou > your choice. My bad, I didn't realize this was a KPart plugin. The Kate screenshot you linked got me confused :p Then yes, a stand-alone package makes sense indeed. > > Cheers > Friedrich > > Cheers, Elvis
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Hi Elvis, Am Montag, 17. August 2020, 22:43:37 CEST schrieb Elvis Angelaccio: > On 15/08/20 19:20, Friedrich W. H. Kossebau wrote: > > Hi, > > > > what is markdownpart you ask? A KParts plugin allowing to view markdown > > documents rendered e.g. in Kate's preview plugin, Ark, Krusader or > > Konqueror, being mainly a wrapper around QTextDocument::setMarkdown & > > QTextBrowser. See here it being used with Kate's preview plugin: > > https://cdn.kde.org/screenshots/markdownpart/markdownpart.png > > Note: for now Qt 5.15-only, 5.14 possible but untested. > > > > I would like to propose markdownpart for a move into community maintenance > > mode and becoming part of release service > > Hi Friedrich, > > why not merge this plugin into kate instead? Let me answer with another question: why with Kate and not with Ark or with Krusader or with Konqueror or any other potential KParts plugin user where Markdown viewing often is useful? Bundling with Kate would be a rather random choice IMHO. And a) make it challenging for packagers to split out an independent packager for the markdown kpart with all the files belonging to it b) make it harder for anyone interested in hacking on it, as they would have to also care about the whole Kate repo and its complete build system, when they just want to improve the markdown viewer e.g. for Ark. But not my call, after all I offer this to KDE community for adoption, sou your choice. Cheers Friedrich
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
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
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
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ć i...@cukic.co, https://cukic.co/ gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B FF04 1C12
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Am Sonntag, 16. August 2020, 00:03:53 CEST schrieb David Faure: > The code looks fine to me. Thanks for review. > The only thing I saw was case-insensitive comparison of the result of > QUrl::scheme() which is unnecessary, it always returns lowercase. Ah, was not aware (and missed the inconsistency with other checks of the scheme). Going to fix the same also in KWebKitPart, WebEnginePart, and KMarkdownWebView, which is the copy trace of that code snippet from what I can tell :) And actually discovered my code had some inverted logic flaw as well, also fixed. > Glad to see KParts still lives :-) Living code fossils, still not ready for static display in museum :) Cheers Friedrich
Re: KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
On samedi 15 août 2020 19:20:53 CEST Friedrich W. H. Kossebau wrote: > Hi, > > what is markdownpart you ask? A KParts plugin allowing to view markdown > documents rendered e.g. in Kate's preview plugin, Ark, Krusader or > Konqueror, being mainly a wrapper around QTextDocument::setMarkdown & > QTextBrowser. See here it being used with Kate's preview plugin: > https://cdn.kde.org/screenshots/markdownpart/markdownpart.png > Note: for now Qt 5.15-only, 5.14 possible but untested. > > I would like to propose markdownpart for a move into community maintenance > mode and becoming part of release service managed projects starting with RS > 20.12. It would match graphics/svgpart in the mode (whose code mode BTW is > aöso similar, mainly a wrapper around QSvgRenderer & QGraphicsView). > > The code lives at https://invent.kde.org/utilities/markdownpart since > yesterday. > I consider the project stable & done already though, given there are no more > features I want myself and given it is based on existing code: mainly a > copy of KMarkdownWebView, which itself was inspired/driven by e.g. KWebKit/ > KWebEngine KParts code. > Some small glitches are with the search tool and incremental search > sometimes, but that might be a bug in QTextDocument/QTextBrowser. > > I have written this plugin mainly because "I could :P" and some people might > like it, less because I want to use this myself everyday (personally > dislike Markdown). So I would be interested to pass maintenance over into > community domain, or interested individuals if there are. Otherwise it > would be a candidate for the attic. > > Today I released a first 0.1.0 version to make it spread and find users who > fancy it: > https://mail.kde.org/pipermail/kde-announce-apps/2020-August/005597.html > > Try yourself by building and installing with "kdesrc-build markdownpart" and > picking "Markdown View (markdownpart)" as preferred embedding plugin for > "text/markdown" in System Settings' File Associations submodule (part of > "Applications"). > > > So I hereby move markdownpart into kdereview mode. > > Please give the code some eyeball times and comment for good and bad or > missing stuff and whether you agree if this should become part of KDE > community-managed set of software. Also hoping to see someone offering > themselves to adopt this project as caring maintainer. > > > BACKGROUND > > You might know the KParts plugin from KMarkdownWebView for the same purpose > (https://invent.kde.org/utilities/kmarkdownwebview). > > It had been written 3 years ago as quick solution, with an important > statement in the README: > "The software should serve as intermediate solution until some native > Qt-based implementation is done." > > While there has been some native variant present meanwhile via the Okular > Markdown support for some time, and available via the Okular KParts plugin, > the paged display of Okular might not meet the desire of some to see the > Markdown document rendered as single adaptive page. > > With QTextDocument having gotten some native markdown-parsing support in Qt > 5.14, some recent discussion about whether to include KMarkdownWebView (and > the QWeb* dependencies it brings) into Kate app bundles for the preview > feature triggered me to give it a try to write a QTextDocument-based > variant. Which turned out to be simple to do in an evening, and so here we > are. The code looks fine to me. The only thing I saw was case-insensitive comparison of the result of QUrl::scheme() which is unnecessary, it always returns lowercase. Glad to see KParts still lives :-) -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
KDEREVIEW: Proposing utilities/markdownpart to become community/release-service-managed
Hi, what is markdownpart you ask? A KParts plugin allowing to view markdown documents rendered e.g. in Kate's preview plugin, Ark, Krusader or Konqueror, being mainly a wrapper around QTextDocument::setMarkdown & QTextBrowser. See here it being used with Kate's preview plugin: https://cdn.kde.org/screenshots/markdownpart/markdownpart.png Note: for now Qt 5.15-only, 5.14 possible but untested. I would like to propose markdownpart for a move into community maintenance mode and becoming part of release service managed projects starting with RS 20.12. It would match graphics/svgpart in the mode (whose code mode BTW is aöso similar, mainly a wrapper around QSvgRenderer & QGraphicsView). The code lives at https://invent.kde.org/utilities/markdownpart since yesterday. I consider the project stable & done already though, given there are no more features I want myself and given it is based on existing code: mainly a copy of KMarkdownWebView, which itself was inspired/driven by e.g. KWebKit/ KWebEngine KParts code. Some small glitches are with the search tool and incremental search sometimes, but that might be a bug in QTextDocument/QTextBrowser. I have written this plugin mainly because "I could :P" and some people might like it, less because I want to use this myself everyday (personally dislike Markdown). So I would be interested to pass maintenance over into community domain, or interested individuals if there are. Otherwise it would be a candidate for the attic. Today I released a first 0.1.0 version to make it spread and find users who fancy it: https://mail.kde.org/pipermail/kde-announce-apps/2020-August/005597.html Try yourself by building and installing with "kdesrc-build markdownpart" and picking "Markdown View (markdownpart)" as preferred embedding plugin for "text/markdown" in System Settings' File Associations submodule (part of "Applications"). So I hereby move markdownpart into kdereview mode. Please give the code some eyeball times and comment for good and bad or missing stuff and whether you agree if this should become part of KDE community-managed set of software. Also hoping to see someone offering themselves to adopt this project as caring maintainer. BACKGROUND You might know the KParts plugin from KMarkdownWebView for the same purpose (https://invent.kde.org/utilities/kmarkdownwebview). It had been written 3 years ago as quick solution, with an important statement in the README: "The software should serve as intermediate solution until some native Qt-based implementation is done." While there has been some native variant present meanwhile via the Okular Markdown support for some time, and available via the Okular KParts plugin, the paged display of Okular might not meet the desire of some to see the Markdown document rendered as single adaptive page. With QTextDocument having gotten some native markdown-parsing support in Qt 5.14, some recent discussion about whether to include KMarkdownWebView (and the QWeb* dependencies it brings) into Kate app bundles for the preview feature triggered me to give it a try to write a QTextDocument-based variant. Which turned out to be simple to do in an evening, and so here we are. Cheers Friedrich
