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
