ghostwriter is ready for your review

[Megan]

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.

> 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?  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
>

-- 
Megan Conkle
ghostwriter developer