Re: KSvg in kdereview
I opened an issue in line with the new process https://invent.kde.org/frameworks/ksvg/-/issues/2 Jonathan On Thu, 20 Apr 2023 at 09:26, Marco Martin wrote: > Hi all, > A part of plasma-framewrok, which is the one to do SVG-based themes, > has now been splitted in a standalone library which is intended to be > a new framework in KF6 (all usages of the plasma-framework version > will be ported once this officially lands, and then those classes will > be removed) > The repo for now lives in > https://invent.kde.org/libraries/plasmasvg > > In the end it will be renamed in ksvg > > Comments? reviews? > > > -- > Marco Martin >
Re: KSvg in kdereview
FWIW the code itself is almost entirely just moved verbatim from plasma-framework, which is already a framework. Nate On 6/21/23 12:41, Friedrich W. H. Kossebau wrote: Am Mittwoch, 21. Juni 2023, 12:23:55 CEST schrieb Ben Cooksley: On Wed, Jun 21, 2023 at 10:12 PM Harald Sitter wrote: LGTM now +2 On Wed, Jun 21, 2023 at 10:04 AM Marco Martin wrote: I fixed CI, passes now Thanks for correcting that. As Friedrich raised the initial concerns it would be nice to have him confirm that the code quality issues he found have all been corrected. Fear I had just superficially looked at things, given I am currently not a stakeholder in this library, no API consumer or contributor. The cmake issues I saw at the time I had fixed directly, anything C++ etc. I had not really looked at, just saw the TODOs and skipped ;) So cannot compare and would have no time reserved here to take a closer look now, others have I assume :) The other thing that stood out was the outdated docs, but that seems to have been fixed/improved on a quick glance +1 The other comment was about the name, but naming, the joy :) ... and people using it/working on it seem fine with the current one, so... Cheers Friedrich
Re: KSvg in kdereview
Am Mittwoch, 21. Juni 2023, 12:23:55 CEST schrieb Ben Cooksley: > On Wed, Jun 21, 2023 at 10:12 PM Harald Sitter wrote: > > LGTM now +2 > > > > On Wed, Jun 21, 2023 at 10:04 AM Marco Martin wrote: > > > I fixed CI, passes now > > Thanks for correcting that. > > As Friedrich raised the initial concerns it would be nice to have him > confirm that the code quality issues he found have all been corrected. Fear I had just superficially looked at things, given I am currently not a stakeholder in this library, no API consumer or contributor. The cmake issues I saw at the time I had fixed directly, anything C++ etc. I had not really looked at, just saw the TODOs and skipped ;) So cannot compare and would have no time reserved here to take a closer look now, others have I assume :) The other thing that stood out was the outdated docs, but that seems to have been fixed/improved on a quick glance +1 The other comment was about the name, but naming, the joy :) ... and people using it/working on it seem fine with the current one, so... Cheers Friedrich
Re: KSvg in kdereview
On Wed, Jun 21, 2023 at 10:12 PM Harald Sitter wrote: > LGTM now +2 > > On Wed, Jun 21, 2023 at 10:04 AM Marco Martin wrote: > > > > I fixed CI, passes now > Thanks for correcting that. As Friedrich raised the initial concerns it would be nice to have him confirm that the code quality issues he found have all been corrected. Note that having something step into Frameworks is a big deal, so this is something that should not be rushed and if there are issues with the code we should fix them sooner vs. later. Thanks, Ben > > > > On Tue, Jun 20, 2023 at 8:44 PM Ben Cooksley wrote: > > > > > > Hi all, > > > > > > Sysadmin has just received a request to move this repository to > Frameworks, however after seeing some of the comments raised here regarding > the repository I took a look myself to see if they had been corrected. > > > > > > At this time CI is still broken in KSvg, for both platforms - > something that was mentioned in an earlier email here. > > > This does not inspire confidence that the code is up to scratch. > > > > > > I've therefore declined to move it to Frameworks at this time. > > > > > > Would be appreciated, given this is looking to be promoted to > Frameworks, if people could please have a further look at this repository > and comment as appropriate. > > > > > > Thanks, > > > Ben > > > > > > On Wed, Jun 14, 2023 at 9:12 PM Marco Martin > wrote: > > >> > > >> Hi all, > > >> Some time has passes, and changes have been done in the repo to > > >> address some of the points. > > >> Now there are review requests in plasma-framework which depends on > > >> this repo (and accompanying plasma-workspace, plasma-desktop etc) > > >> It would probably be better to have this in frameworks to have the > > >> rest depending from it? > > >> > > >> On Thu, Apr 20, 2023 at 10:25 AM Marco Martin > wrote: > > >> > > > >> > Hi all, > > >> > A part of plasma-framewrok, which is the one to do SVG-based themes, > > >> > has now been splitted in a standalone library which is intended to > be > > >> > a new framework in KF6 (all usages of the plasma-framework version > > >> > will be ported once this officially lands, and then those classes > will > > >> > be removed) > > >> > The repo for now lives in > > >> > https://invent.kde.org/libraries/plasmasvg > > >> > > > >> > In the end it will be renamed in ksvg > > >> > > > >> > Comments? reviews? > > >> > > > >> > > > >> > -- > > >> > Marco Martin > > >> > > >> -- > > >> Marco Martin >
Re: KSvg in kdereview
LGTM now +2 On Wed, Jun 21, 2023 at 10:04 AM Marco Martin wrote: > > I fixed CI, passes now > > On Tue, Jun 20, 2023 at 8:44 PM Ben Cooksley wrote: > > > > Hi all, > > > > Sysadmin has just received a request to move this repository to Frameworks, > > however after seeing some of the comments raised here regarding the > > repository I took a look myself to see if they had been corrected. > > > > At this time CI is still broken in KSvg, for both platforms - something > > that was mentioned in an earlier email here. > > This does not inspire confidence that the code is up to scratch. > > > > I've therefore declined to move it to Frameworks at this time. > > > > Would be appreciated, given this is looking to be promoted to Frameworks, > > if people could please have a further look at this repository and comment > > as appropriate. > > > > Thanks, > > Ben > > > > On Wed, Jun 14, 2023 at 9:12 PM Marco Martin wrote: > >> > >> Hi all, > >> Some time has passes, and changes have been done in the repo to > >> address some of the points. > >> Now there are review requests in plasma-framework which depends on > >> this repo (and accompanying plasma-workspace, plasma-desktop etc) > >> It would probably be better to have this in frameworks to have the > >> rest depending from it? > >> > >> On Thu, Apr 20, 2023 at 10:25 AM Marco Martin wrote: > >> > > >> > Hi all, > >> > A part of plasma-framewrok, which is the one to do SVG-based themes, > >> > has now been splitted in a standalone library which is intended to be > >> > a new framework in KF6 (all usages of the plasma-framework version > >> > will be ported once this officially lands, and then those classes will > >> > be removed) > >> > The repo for now lives in > >> > https://invent.kde.org/libraries/plasmasvg > >> > > >> > In the end it will be renamed in ksvg > >> > > >> > Comments? reviews? > >> > > >> > > >> > -- > >> > Marco Martin > >> > >> -- > >> Marco Martin
Re: KSvg in kdereview
I fixed CI, passes now On Tue, Jun 20, 2023 at 8:44 PM Ben Cooksley wrote: > > Hi all, > > Sysadmin has just received a request to move this repository to Frameworks, > however after seeing some of the comments raised here regarding the > repository I took a look myself to see if they had been corrected. > > At this time CI is still broken in KSvg, for both platforms - something that > was mentioned in an earlier email here. > This does not inspire confidence that the code is up to scratch. > > I've therefore declined to move it to Frameworks at this time. > > Would be appreciated, given this is looking to be promoted to Frameworks, if > people could please have a further look at this repository and comment as > appropriate. > > Thanks, > Ben > > On Wed, Jun 14, 2023 at 9:12 PM Marco Martin wrote: >> >> Hi all, >> Some time has passes, and changes have been done in the repo to >> address some of the points. >> Now there are review requests in plasma-framework which depends on >> this repo (and accompanying plasma-workspace, plasma-desktop etc) >> It would probably be better to have this in frameworks to have the >> rest depending from it? >> >> On Thu, Apr 20, 2023 at 10:25 AM Marco Martin wrote: >> > >> > Hi all, >> > A part of plasma-framewrok, which is the one to do SVG-based themes, >> > has now been splitted in a standalone library which is intended to be >> > a new framework in KF6 (all usages of the plasma-framework version >> > will be ported once this officially lands, and then those classes will >> > be removed) >> > The repo for now lives in >> > https://invent.kde.org/libraries/plasmasvg >> > >> > In the end it will be renamed in ksvg >> > >> > Comments? reviews? >> > >> > >> > -- >> > Marco Martin >> >> -- >> Marco Martin
Re: KSvg in kdereview
Hi all, Sysadmin has just received a request to move this repository to Frameworks, however after seeing some of the comments raised here regarding the repository I took a look myself to see if they had been corrected. At this time CI is still broken in KSvg, for both platforms - something that was mentioned in an earlier email here. This does not inspire confidence that the code is up to scratch. I've therefore declined to move it to Frameworks at this time. Would be appreciated, given this is looking to be promoted to Frameworks, if people could please have a further look at this repository and comment as appropriate. Thanks, Ben On Wed, Jun 14, 2023 at 9:12 PM Marco Martin wrote: > Hi all, > Some time has passes, and changes have been done in the repo to > address some of the points. > Now there are review requests in plasma-framework which depends on > this repo (and accompanying plasma-workspace, plasma-desktop etc) > It would probably be better to have this in frameworks to have the > rest depending from it? > > On Thu, Apr 20, 2023 at 10:25 AM Marco Martin wrote: > > > > Hi all, > > A part of plasma-framewrok, which is the one to do SVG-based themes, > > has now been splitted in a standalone library which is intended to be > > a new framework in KF6 (all usages of the plasma-framework version > > will be ported once this officially lands, and then those classes will > > be removed) > > The repo for now lives in > > https://invent.kde.org/libraries/plasmasvg > > > > In the end it will be renamed in ksvg > > > > Comments? reviews? > > > > > > -- > > Marco Martin > > -- > Marco Martin >
Re: KSvg in kdereview
Hi all, Some time has passes, and changes have been done in the repo to address some of the points. Now there are review requests in plasma-framework which depends on this repo (and accompanying plasma-workspace, plasma-desktop etc) It would probably be better to have this in frameworks to have the rest depending from it? On Thu, Apr 20, 2023 at 10:25 AM Marco Martin wrote: > > Hi all, > A part of plasma-framewrok, which is the one to do SVG-based themes, > has now been splitted in a standalone library which is intended to be > a new framework in KF6 (all usages of the plasma-framework version > will be ported once this officially lands, and then those classes will > be removed) > The repo for now lives in > https://invent.kde.org/libraries/plasmasvg > > In the end it will be renamed in ksvg > > Comments? reviews? > > > -- > Marco Martin -- Marco Martin
Re: KSvg in kdereview
On Fri, May 12, 2023 at 3:41 PM Friedrich W. H. Kossebau wrote: > Did some small fixes (library e.g. was installed with literal ".SOVERSION" as > suffix...), but the more I did and more I saw... IMHO this should not have yet > been passed to kdereview, being in alpha state for my taste. ouch, sorry :/ and thanks one thing that will make the landing even longer is that at the plasma sprint we decided it was the case for making it depend from a new (also not yet landed) framework (the new home for KColorScheme) so it will still take more time. > E.g. "TODO KF6" ideally would not be handled during the review phase, but > before. there was still one todo kf6 which after some tought and discussion was then removed as doing that it would cause a regression in themes we don't want. > And the README and other docs not (yet) mentioning what the scope & purpose of > the library, but being dead copies from Plasma Framwworks also makes things > harder to assess. ok, i'll write a more comprehensive introduction there > Builds on CI all failing ever since and before also looks a bit unattractive. > Currently still with FreeBSD. fixed it now > For the name, "KSvg" sounds very unspecifc to me, ideally the name would > reflect the purpose & scope of the library some more (but then that is not > defined somewhere also, so... ;) ). Something with SVG, but what exactly? > Perhaps "KSvgTheme" (proposed by Sune on irc) or similar might make it more > clear? I'm not sure, compared to the plasma version the "theming" part has been scaled down a bit (so that the "theme" class becaume "imageset" as most of the kcolorscheme wrapping api is gone) I'm not opposed to changing the name again, but not sure what other names could actually be more clear. so, basically the things that does over qsvg (the why for using qtsvg not directly) are: * disk caching of the rendered pixmaps to have things faster * stylesheet based recoloring * the fact that svgs can come in bundles (the theming part, tough is not the main thing, so i would not give the name of the whole library after that) * the 9 patch framesvg for doing rectangular things * qml bindings > Also using "KSvg" as namespace results in class names like KSvg::;Svg, > KSvg::SvgItem, etc. which looks a bit strange to my eyes on consumer side due > to the duplication. In my perfect world the naming would see some overhaul > given this is a new library. Yes, some one-time porting pain, but sanity > afterwards, for a certain type of sustainability ;) one thing is that the library is really about svgs and nothing else, and the Svg class is about rendering one single svg, so I don't see many ways around about both contianing "svg" ? (I was aware there was an old kde3 ksvg library, though talking about it in some tuesday meeting didn't seem to be an issue) Open to ideas :) -- Marco Martin
Re: KSvg in kdereview
Hi, Am Donnerstag, 20. April 2023, 10:25:34 CEST schrieb Marco Martin: > Hi all, > A part of plasma-framewrok, which is the one to do SVG-based themes, > has now been splitted in a standalone library which is intended to be > a new framework in KF6 (all usages of the plasma-framework version > will be ported once this officially lands, and then those classes will > be removed) > The repo for now lives in > https://invent.kde.org/libraries/plasmasvg > > In the end it will be renamed in ksvg > > Comments? reviews? Came across the library yesterday by chance. Did some small fixes (library e.g. was installed with literal ".SOVERSION" as suffix...), but the more I did and more I saw... IMHO this should not have yet been passed to kdereview, being in alpha state for my taste. E.g. "TODO KF6" ideally would not be handled during the review phase, but before. And the README and other docs not (yet) mentioning what the scope & purpose of the library, but being dead copies from Plasma Framwworks also makes things harder to assess. Builds on CI all failing ever since and before also looks a bit unattractive. Currently still with FreeBSD. For the name, "KSvg" sounds very unspecifc to me, ideally the name would reflect the purpose & scope of the library some more (but then that is not defined somewhere also, so... ;) ). Something with SVG, but what exactly? Perhaps "KSvgTheme" (proposed by Sune on irc) or similar might make it more clear? Also using "KSvg" as namespace results in class names like KSvg::;Svg, KSvg::SvgItem, etc. which looks a bit strange to my eyes on consumer side due to the duplication. In my perfect world the naming would see some overhaul given this is a new library. Yes, some one-time porting pain, but sanity afterwards, for a certain type of sustainability ;) My 2 cents as someone who just came by, but with no current own needs in that library. Cheers Friedrich
Re: KSvg in kdereview
On Sun, Apr 23, 2023 at 11:05 AM Albert Astals Cid wrote: > > Comments? reviews? > > There's a few "TODO KF6" in the code, should those be addressed? ok, I'll go over all of them in the review period > Is the plan to make it less plasma-specific with the rename? > > i.e. the ImageSet constructor says > > Default constructor. It will be the global theme configured in plasmarc > > Is that wanted or not (i've no opinion, just asking to make sure since the > rename seems to want to make it "less plasma"). hm, it looks like there is still some stray documentation which doesn't completely reflect truth anymore (right now it doesn't use that config file anymore, it's up tp the user's code) will correct -- Marco Martin
Re: KSvg in kdereview
El dijous, 20 d’abril de 2023, a les 10:25:34 (CEST), Marco Martin va escriure: > Hi all, > A part of plasma-framewrok, which is the one to do SVG-based themes, > has now been splitted in a standalone library which is intended to be > a new framework in KF6 (all usages of the plasma-framework version > will be ported once this officially lands, and then those classes will > be removed) > The repo for now lives in > https://invent.kde.org/libraries/plasmasvg > > In the end it will be renamed in ksvg > > Comments? reviews? The README.md needs to be updated i think. Cheers, Albert
Re: KSvg in kdereview
El dijous, 20 d’abril de 2023, a les 10:25:34 (CEST), Marco Martin va escriure: > Hi all, > A part of plasma-framewrok, which is the one to do SVG-based themes, > has now been splitted in a standalone library which is intended to be > a new framework in KF6 (all usages of the plasma-framework version > will be ported once this officially lands, and then those classes will > be removed) > The repo for now lives in > https://invent.kde.org/libraries/plasmasvg > > In the end it will be renamed in ksvg > > Comments? reviews? There's a few "TODO KF6" in the code, should those be addressed? Is the plan to make it less plasma-specific with the rename? i.e. the ImageSet constructor says Default constructor. It will be the global theme configured in plasmarc Is that wanted or not (i've no opinion, just asking to make sure since the rename seems to want to make it "less plasma"). Cheers, Albert
KSvg in kdereview
Hi all, A part of plasma-framewrok, which is the one to do SVG-based themes, has now been splitted in a standalone library which is intended to be a new framework in KF6 (all usages of the plasma-framework version will be ported once this officially lands, and then those classes will be removed) The repo for now lives in https://invent.kde.org/libraries/plasmasvg In the end it will be renamed in ksvg Comments? reviews? -- Marco Martin
