ghostwriter is ready for your review

[Albert Astals Cid]

El diumenge, 16 d’octubre de 2022, a les 5:05:40 (CEST), Megan va escriure:
> Hi Albert,
> 
> > If you're going to change something as core to an app as the i18n system I
> > wouldn't say we're done for review stage yet. Or maybe you don't need to
> > change it?
> 
> Carl said it should be enough to pass review with a Messages.sh and
> including ECMPoQmTools in the CMakeLists.txt file.  Also, he noted that
> other apps are still using QtLinguist, such as Analitza and Stopmotion.

Using Qt for translations is fine acceptable.

Doing a "major" rewrite after review kind of invalidates the review, and 
that's why I say that if you're going to rewrite it we should do the review 
later.

> 
> > I have lots of actions with the ¿same icon? is that supposed to be like
> > that or we just don't have those in
> > breeze?https://i.imgur.com/m2Ytkcc.png
> It's definitely not supposed to look like that.  I tried a fresh install
> on my machine (removing and rebuilding from scratch) but could not
> replicate the issue.  It's supposed to be using Font Awesome's font
> glyphs for the icons, since they are easily styled along with the normal
> text in QSS/CSS.  I also double checked that I don't have Font Awesome
> installed as a font.  Weird.
> 
> > You have both a CMakeLists and a qmake pro. I'd suggest to remove one,
> > specially the pro one, maintaining 2 build systems will only bring you
> > sadness.
> 
> Agreed.  I had forgotten to remove it. It's gone now.  Thanks!
> 
> > You have 4 libs in the 3rdparty folder, is there any chance to use actual
> > dependencies and not copied code?
> 
> 1. Unfortunately, some of the dependencies aren't in every GNU/Linux
> distribution.
> 2. It is easier for doing Windows and MacOS builds to have the
> dependencies bundled with the app code.
> 3. To protect against sudden API changes across distros, it's best to
> freeze the versions of the dependencies needed by keeping them bundled. 
> This way I can upgrade them when I'm prepared to rather than as an
> emergency fix.
> 
> > Using KXmlGui would maybe make sense?
> 
> Yes, I may very well do that in the future.  It looks very useful.
> 
> > When running i get
> > 
> >    Command "pandoc" is not available.
> >    Command "multimarkdown" is not available.
> >    Command "cmark" is not available.
> > 
> > on the command line, "normal" users will start the app via the menu and
> > won't get to see that.
> 
> That's largely for my own use so I can extract better information from
> people reporting issues in the bug tracker.  I would like to move this
> to the About dialog in the future.  I agree that would be better.  (But
> can I do that later and still pass review?)
> 
> > The theme choose dialog only has "Close", i'm guess that "ok" would make
> > more sense given that it sets the new theme when pressing it? I may be
> > wrong there, so feel free to disagree (well here and with everything :D)
> 
> Yes, "OK" does give the sense that your changes will take place. On the
> other hand, it might also convey to the user that closing the window
> from the upper corner will cancel the changes.  I wouldn't want to give
> that false impression.  But if you still feel that "OK" is better, I
> will change it there and in the Preferences dialog.  :)
> 
> > For some reason it thinks my language is the valencian variant of catalan,
> > when it should be non-variant catalan one.
> 
> I will look into this and come up with a fix.  :)
> 
> > There seems to be a huge memory leak regarding sonnet and CmarkGfm usage,
> > see what valgrind tells mehttps://pst.klgrth.io/paste/wc6vn
> 
> For the CmarkGfm memory leak report, I wonder if the creation and return
> of a new MarkdownAST is the cause?  This class is handed off to the
> MarkdownDocument class, which handles deleting it when either a new one
> is set or its destructor is called.  Maybe valgrind isn't smart enough
> to notice that?  

VERY unlikely that you found a valgrind bug.

https://invent.kde.org/office/ghostwriter/-/merge_requests/8

Cheers,
  Albert

P.S: The amount of this-> in the code is very much non customary in C++ code. 
Not wrong but feels itchy :D

> I really don't know.  Do you have any insight into this?
> 
> As for the Sonnet usage, that almost looks like Hunspell is the cause?
> 
> I ran valgrind myself, and I see Sonnet/Hunspell issue. Strangely I
> don't see the CmarkGfm one. (But I see plenty for the AppSettings
> singleton class, which makes sense considering it shouldn't be destroyed
> until the application exits.)
> 
> Thanks for the quick feedback!
> 
> Megan
> 
> On 10/15/22 15:16, Albert Astals Cid wrote:
> > El dijous, 13 d’octubre de 2022, a les 8:52:26 (CEST), Megan va escriure:
> >> Hello everyone,
> >> 
> >> The /ghostwriter/ Markdown editor has finally hatched from its
> >> incubation and is ready for you to review at your convenience.
> >> 
> >> Project repo:https://invent.kde.org/office/ghostwriter
> >> 
> >> Project website:https://ghostwriter.kde.org
> >> 
> >> Note: ghostwriter currently uses QtLinguist for translations. However, I
> >> plan to transition it to ki18n as soon as I can.  Any help you can
> >> provide would be appreciated, of course!
> > 
> > If you're going to change something as core to an app as the i18n system I
> > wouldn't say we're done for review stage yet. Or maybe you don't need to
> > change it?
> > 
> > anyhow here's my quick look
> > 
> > I have lots of actions with the ¿same icon? is that supposed to be like
> > that or we just don't have those in
> > breeze?https://i.imgur.com/m2Ytkcc.png
> > 
> > You have both a CMakeLists and a qmake pro. I'd suggest to remove one,
> > specially the pro one, maintaining 2 build systems will only bring you
> > sadness.
> > 
> > You have 4 libs in the 3rdparty folder, is there any chance to use actual
> > dependencies and not copied code?
> > 
> > Using KXmlGui would maybe make sense?
> > 
> > When running i get
> > 
> >    Command "pandoc" is not available.
> >    Command "multimarkdown" is not available.
> >    Command "cmark" is not available.
> > 
> > on the command line, "normal" users will start the app via the menu and
> > won't get to see that.
> > 
> > 
> > The theme choose dialog only has "Close", i'm guess that "ok" would make
> > more sense given that it sets the new theme when pressing it? I may be
> > wrong there, so feel free to disagree (well here and with everything :D)
> > 
> > For some reason it thinks my language is the valencian variant of catalan,
> > when it should be non-variant catalan one.
> > 
> > There seems to be a huge memory leak regarding sonnet and CmarkGfm usage,
> > see what valgrind tells mehttps://pst.klgrth.io/paste/wc6vn
> > 
> > Cheers,
> > 
> >    Albert
> >> 
> >> Thanks so much!
> >> 
> >> Megan