ghostwriter is ready for your review

[Megan]

> 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.
Oh, I see.  I was under the impression that I had to eventually use 
ki18n.  As long as I don't have to switch to ki18n later, then I suppose 
I'll just stay with Qt Linguist, so long as you are okay with it.

> VERY unlikely that you found a valgrind bug.
>
> https://invent.kde.org/office/ghostwriter/-/merge_requests/8
Thank you for that!  I wasn't sure how to fix it.  Please forgive my 
inexperience with reading valgrind reports.  :)

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

Yeah, I know, but I found that sticking "m_" in front of every class 
member feels itchy to me.  :D
Though I suppose it would be better not to use this-> in front of method 
calls in the future.  :)

One last thing, I believe the issue with the Catalan incorrectly showing 
the Valencia variant on your system should be fixed now.  I also updated 
the code to look at the $LANGUAGE environment variable to ensure 
Valencia and other language variants are detected if they are listed first.

Thanks again!

On 10/17/22 14:19, Albert Astals Cid wrote:
> 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
>
>
>

-- 
Megan Conkle
ghostwriter developer