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

Reply via email to