Re: KQuickImageEditor in KDEReview
Le jeudi, mars 25, 2021 4:47 PM, Harald Sitter a écrit :
> - really close to 100% reuse cover. plz consider adding data to
> KQuickImageEditorConfig.cmake.in and src/controls/plugins.qmltypes
>
> - in fact... shouldn't the qmltypes file be generated at build time?
> - there's commented out KDEFrameworkCompilerSettings in CMakeLists,
> please remove it, Friedrich asked us to not use it outside frameworks
> and having it commented out only tempts people into using it ;)
>
> - as with koko, it might make sense to bump the kf5 requirement to
> 5.79 and use clang format+hooks. there are some stilistic
> inconsistency within the code already
>
> - you should consider adding an option() definition for the
> BUILD_SHARED_LIBS variable so it is documented and expicitly
> initialized to a default value, I guess
>
> - ResizeRectangle has raw pointers that aren't initialized to nullptr by
> default
> - resizerectangle.cpp includes moc_resizerectangle.cpp is there a
> reason for that? shouldn't automoc magic do this on its own?
>
> - same for resizehandle.cpp
>
> some typos in imagedocument.h:
> Mirrror
> horizonal (actually in mirrocommand.{h,cpp} as well)
> operation
Thanks for the review and sorry for the delay. I just made the changes :)
>
> HS
>
Re: KQuickImageEditor in KDEReview
- really close to 100% reuse cover. plz consider adding data to
KQuickImageEditorConfig.cmake.in and src/controls/plugins.qmltypes
- in fact... shouldn't the qmltypes file be generated at build time?
- there's commented out KDEFrameworkCompilerSettings in CMakeLists,
please remove it, Friedrich asked us to not use it outside frameworks
and having it commented out only tempts people into using it ;)
- as with koko, it might make sense to bump the kf5 requirement to
5.79 and use clang format+hooks. there are some stilistic
inconsistency within the code already
- you should consider adding an option() definition for the
BUILD_SHARED_LIBS variable so it is documented and expicitly
initialized to a default value, I guess
- ResizeRectangle has raw pointers that aren't initialized to nullptr by default
- resizerectangle.cpp includes moc_resizerectangle.cpp is there a
reason for that? shouldn't automoc magic do this on its own?
- same for resizehandle.cpp
some typos in imagedocument.h:
Mirrror
horizonal (actually in mirrocommand.{h,cpp} as well)
operattion
HS
Re: KQuickImageEditor in KDEReview
El dissabte, 13 de març de 2021, a les 23:10:11 CET, Carl Schwan va escriure: > Hello again, > I putting KQuickImageEditor in KDEReview. KQuickImageEditor is a very > simple image editor for QtQuick projects. It is designed to not depends > on Kirigami (but a simple example that integrate in Kirigami application > is provided in the source code). It doesn't have any translations in it > and just focus on image manipulation. Remove ecm_create_qm_loader if you're not using qm files Your plugin.qmltypes seems like needs updating. Cheers, Albert > It's currently used by NeoChat, Pix > and Koko and packaged in all the major distributions (Fedora, openSUSE, Arch, > Neon). > > I currently advice against using it on your own project since I'm not > completely happy with the current backend and will probably make change > to the API. > > Repo: https://invent.kde.org/libraries/kquickimageeditor/ > Regards, > Carl >
Re: KQuickImageEditor in KDEReview
Le samedi, mars 13, 2021 11:33 PM, Albert Astals Cid a écrit : > El dissabte, 13 de març de 2021, a les 23:10:11 CET, Carl Schwan va escriure: > > > Hello again, > > I putting KQuickImageEditor in KDEReview. KQuickImageEditor is a very > > simple image editor for QtQuick projects. It is designed to not depends > > on Kirigami (but a simple example that integrate in Kirigami application > > is provided in the source code). It doesn't have any translations in it > > and just focus on image manipulation. It's currently used by NeoChat, Pix > > and Koko and packaged in all the major distributions (Fedora, openSUSE, > > Arch, > > Neon). > > I currently advice against using it on your own project since I'm not > > completely happy with the current backend and will probably make change > > to the API. > > Am I understand this email correctly? > > It seems you say "please review this; I'll rewrite it shortly". > > Maybe we should wait to do the review after the rewrite? It seems that making KQuickImageEditor pass KDEReview is a requirement for Koko KDEReview process and since I want to have a stable release of Koko in the next Pinephone batch, I would really appreciate a review. Also just to clarify KQuickImageEditor currently works for simple image manipulation like rotation, cropping, mirroring an image. The problem is more that the current back-end limits the possibility of image editing to only these simple operations. This is enough to almost reach feature parity with Gwenview own image editor (that was my goal when I first started this project) but won't make it possible to apply filters and other cool effects that many except for an integrated image editor in chat applications on mobile for example. Cheers, Carl > Cheers, > Albert > > > Repo: https://invent.kde.org/libraries/kquickimageeditor/ > > Regards, > > Carl
Re: KQuickImageEditor in KDEReview
El dissabte, 13 de març de 2021, a les 23:10:11 CET, Carl Schwan va escriure: > Hello again, > I putting KQuickImageEditor in KDEReview. KQuickImageEditor is a very > simple image editor for QtQuick projects. It is designed to not depends > on Kirigami (but a simple example that integrate in Kirigami application > is provided in the source code). It doesn't have any translations in it > and just focus on image manipulation. It's currently used by NeoChat, Pix > and Koko and packaged in all the major distributions (Fedora, openSUSE, Arch, > Neon). > > I currently advice against using it on your own project since I'm not > completely happy with the current backend and will probably make change > to the API. Am I understand this email correctly? It seems you say "please review this; I'll rewrite it shortly". Maybe we should wait to do the review after the rewrite? Cheers, Albert > > Repo: https://invent.kde.org/libraries/kquickimageeditor/ > Regards, > Carl >
KQuickImageEditor in KDEReview
Hello again, I putting KQuickImageEditor in KDEReview. KQuickImageEditor is a very simple image editor for QtQuick projects. It is designed to not depends on Kirigami (but a simple example that integrate in Kirigami application is provided in the source code). It doesn't have any translations in it and just focus on image manipulation. It's currently used by NeoChat, Pix and Koko and packaged in all the major distributions (Fedora, openSUSE, Arch, Neon). I currently advice against using it on your own project since I'm not completely happy with the current backend and will probably make change to the API. Repo: https://invent.kde.org/libraries/kquickimageeditor/ Regards, Carl
