Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review24335 --- This review has been submitted with commit 01b92b2399d80eaacf46d1a11dcbf521a1816c17 by Giorgos Tsiapaliokas to branch master. - Commit Hook On Dec. 23, 2012, 3:23 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 23, 2012, 3:23 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review24243 --- Ship it! Ship It! - Aaron J. Seigo On Dec. 23, 2012, 3:23 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 23, 2012, 3:23 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 23, 2012, 3:23 p.m.) Review request for Plasma. Changes --- Thinking about it some more, all named files should be shown. yes and it works :) The themes are working correct and also there was one more file like the colors. It was the mainconfigui, so I have added it in the ui like the main.xml. If the mainconfigui doesn't exist add it in the top entries else add it where it belongs. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs (updated) - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
On Nov. 29, 2012, 1:53 p.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, line 544 http://git.reviewboard.kde.org/r/106680/diff/5/?file=93807#file93807line544 Plasma/Theme should not appear in this file. Giorgos Tsiapaliokas wrote: the colors file isn't a requiredDirectory nor a requiredFile, so If I don't add it by hand in the m_topEntries it won't be visible and also we want the colors file in the m_topEntries only when the package is a theme. No? What am I missing? Aaron J. Seigo wrote: perhaps add an exception for top level non-required files, so that everything in the top level, required or not, is shown. Giorgos Tsiapaliokas wrote: I can't find a way (I don't think that there is one) in order to specify which files are top level ones. Before the refactor of the packagemodel, the model was doing this: * find all the files(required or not) * check if you can give them a name and call them namedFiles(remove the ones which have a name from the hash) * everything that has been left in the hash add it as a top level item Now with the refactor of the packagemodel, it does: * put the required files in a hash and all(including the required ones) the files in another one ** We need the second hash with all the files, because if the project already exists, we want to add in the ui(with names) all the existing files not just the required ones. * add all the required files in the ui and give them a name (namedFiles) * if there are requiredFiles still in the hash add them in the ui. I see your point that adding a hard coded key can make plasmate fail at some point, but those keys doesn't change often(actually they don't change at all. No?) so we are fine(not ok). Even if plasmate fails, in the -worst/and only- case scenario the user won't be able see/modify the colors file Thinking about it some more, all named files should be shown. Everything that is not a directory but a key with a single file associated with it should be shown, otherwise any package structure that has a file in the same way ThemePackage has colors will fail. - Aaron J. --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review22754 --- On Dec. 7, 2012, 5:58 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 7, 2012, 5:58 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
On Nov. 29, 2012, 1:53 p.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, line 544 http://git.reviewboard.kde.org/r/106680/diff/5/?file=93807#file93807line544 Plasma/Theme should not appear in this file. Giorgos Tsiapaliokas wrote: the colors file isn't a requiredDirectory nor a requiredFile, so If I don't add it by hand in the m_topEntries it won't be visible and also we want the colors file in the m_topEntries only when the package is a theme. No? What am I missing? Aaron J. Seigo wrote: perhaps add an exception for top level non-required files, so that everything in the top level, required or not, is shown. I can't find a way (I don't think that there is one) in order to specify which files are top level ones. Before the refactor of the packagemodel, the model was doing this: * find all the files(required or not) * check if you can give them a name and call them namedFiles(remove the ones which have a name from the hash) * everything that has been left in the hash add it as a top level item Now with the refactor of the packagemodel, it does: * put the required files in a hash and all(including the required ones) the files in another one ** We need the second hash with all the files, because if the project already exists, we want to add in the ui(with names) all the existing files not just the required ones. * add all the required files in the ui and give them a name (namedFiles) * if there are requiredFiles still in the hash add them in the ui. I see your point that adding a hard coded key can make plasmate fail at some point, but those keys doesn't change often(actually they don't change at all. No?) so we are fine(not ok). Even if plasmate fails, in the -worst/and only- case scenario the user won't be able see/modify the colors file On Nov. 29, 2012, 1:53 p.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, lines 196-205 http://git.reviewboard.kde.org/r/106680/diff/5/?file=93807#file93807line196 why not add images, config and animations to m_themeImageDialogOptions and rename it to m_dialogOptions Giorgos Tsiapaliokas wrote: I have put images and config into the hash, but how can it be done for animations too? else if(!qstrcmp(key, animations) packageType() != Plasma/Theme) we check the key and the packageType, so we can't use the hash here. No? Aaron J. Seigo wrote: i guess the real question here is why they are using such different dialogs :) could they be made to use the same dialog? if not, could the switching between the different dialogs be done outside of the PackageModel? there are two files with the name animations the one is in the plasmoidpackage which(the file) is a normal file and the second one is in the themepackage which is an image. So we need two different dialogs. I guess we can decide for the proper dialog in the editpage.cpp but still we would have to do the if (somethingPackageType() != Plasma/Theme) plus that in the editpage.cpp we don't have access to this information, so we have to write more code. :) One possible solution could be to not have two animations keys. We could have a theme_animations and a plasmoid_animations. - Giorgos --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review22754 --- On Dec. 7, 2012, 5:58 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 7, 2012, 5:58 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
On Nov. 29, 2012, 1:53 p.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, lines 196-205 http://git.reviewboard.kde.org/r/106680/diff/5/?file=93807#file93807line196 why not add images, config and animations to m_themeImageDialogOptions and rename it to m_dialogOptions I have put images and config into the hash, but how can it be done for animations too? else if(!qstrcmp(key, animations) packageType() != Plasma/Theme) we check the key and the packageType, so we can't use the hash here. No? On Nov. 29, 2012, 1:53 p.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, line 544 http://git.reviewboard.kde.org/r/106680/diff/5/?file=93807#file93807line544 Plasma/Theme should not appear in this file. the colors file isn't a requiredDirectory nor a requiredFile, so If I don't add it by hand in the m_topEntries it won't be visible and also we want the colors file in the m_topEntries only when the package is a theme. No? What am I missing? - Giorgos --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review22754 --- On Nov. 5, 2012, 5:51 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 5, 2012, 5:51 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Dec. 7, 2012, 5:58 p.m.) Review request for Plasma. Changes --- update the patch Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs (updated) - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review22754 --- plasmate/packagemodel.cpp http://git.reviewboard.kde.org/r/106680/#comment17386 unused? plasmate/packagemodel.cpp http://git.reviewboard.kde.org/r/106680/#comment17385 why not add images, config and animations to m_themeImageDialogOptions and rename it to m_dialogOptions plasmate/packagemodel.cpp http://git.reviewboard.kde.org/r/106680/#comment17387 should not have anything in here specific to a given package type, in this case Plasma/Theme. plasmate/packagemodel.cpp http://git.reviewboard.kde.org/r/106680/#comment17388 Plasma/Theme should not appear in this file. - Aaron J. Seigo On Nov. 5, 2012, 5:51 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 5, 2012, 5:51 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
On Nov. 2, 2012, 10:20 a.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, lines 546-551 http://git.reviewboard.kde.org/r/106680/diff/4/?file=93274#file93274line546 this looks wrong; in the Package definition there is an entry for colors. even if it is not created by default it should still show up in the model. the whole point of this model, after all, is to have a generic way to represent a Plasma::Package. same with the checks for Plasma/Theme in the data() method. none of these should be necessary. Giorgos Tsiapaliokas wrote: I don't understand. this looks wrong; in the Package definition there is an entry for colors. even if it is not created by default it should still show up in the model with the above it will always show the colors in the ui hardcoding package-specific data in the model is wrong. the point of having Package is to have a generic way to describe packages. if we then have to put package-specific hacks into our code, it is failing to do this :) so ... what needs to be done is to find a way to show the colors entry (or any similar top-level item) when a Package is loaded into the model. - Aaron J. --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review21341 --- On Nov. 5, 2012, 5:51 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 5, 2012, 5:51 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 5, 2012, 5:51 p.m.) Review request for Plasma. Changes --- -fixes the issues mentioned above(except from one) -with the use of searchPath, plasmate's packagemodel didn't show the main scripting file correct(various new bugs), fixed Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs (updated) - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
On Nov. 2, 2012, 10:20 a.m., Aaron J. Seigo wrote: plasmate/packagemodel.cpp, lines 546-551 http://git.reviewboard.kde.org/r/106680/diff/4/?file=93274#file93274line546 this looks wrong; in the Package definition there is an entry for colors. even if it is not created by default it should still show up in the model. the whole point of this model, after all, is to have a generic way to represent a Plasma::Package. same with the checks for Plasma/Theme in the data() method. none of these should be necessary. I don't understand. this looks wrong; in the Package definition there is an entry for colors. even if it is not created by default it should still show up in the model with the above it will always show the colors in the ui - Giorgos --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review21341 --- On Nov. 5, 2012, 5:51 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 5, 2012, 5:51 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.h efa3001 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 2, 2012, 8:46 a.m.) Review request for Plasma. Changes --- yesterday in irc, we searched the issue(me and Aaron) and Aaron found out that this isn't a bug in libplasma but in plasmate's packagemodel. Indeed plasmate's packagemodel was using PackageStructure::path(which is deprecated) instead of PackageStructure::searchPath. the issues are solved :) Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs (updated) - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/#review21341
---
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16609
this should be a member of the class and set up in the constructor. data()
gets called often, and creating it over and over here is wasteful
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16607
much faster if written as:
} else if (themeDialogOptions.contains(key)) {
return themeDialogOptions.value(key);
}
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16608
this would also be better with a simple check if key starts with
[plasmate]/themeImageDialog/. it could even be combined with the line below:
const char *imagePrefix = [plasmate]/themeImageDialog/;
if (!qstrncmp(key, images) || !qstrncmp(key, imagePrefix,
qstrlen(imagePrefix))) {
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16612
minor tip: tmpPaths can be const
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16611
foreach ( -- missing space :)
plasmate/packagemodel.cpp
http://git.reviewboard.kde.org/r/106680/#comment16610
this looks wrong; in the Package definition there is an entry for colors.
even if it is not created by default it should still show up in the model. the
whole point of this model, after all, is to have a generic way to represent a
Plasma::Package.
same with the checks for Plasma/Theme in the data() method. none of these
should be necessary.
- Aaron J. Seigo
On Nov. 2, 2012, 8:46 a.m., Giorgos Tsiapaliokas wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/
---
(Updated Nov. 2, 2012, 8:46 a.m.)
Review request for Plasma.
Description
---
create a new theme package-click on the new
a file dialog should appear but instead a simple edit box appears requesting
a new filename.
This patch solves the issue
Diffs
-
plasmate/editors/editpage.h 5cb3ea6
plasmate/editors/editpage.cpp 7e82ff2
plasmate/packagemodel.cpp 9eb0914
Diff: http://git.reviewboard.kde.org/r/106680/diff/
Testing
---
Thanks,
Giorgos Tsiapaliokas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 1, 2012, 3:08 p.m.) Review request for Plasma. Changes --- I have fixed the above issues. There is one more issue, plasmate doesn't add a the description for the *svgz files. I have searched the issue and I noticed that issue comes from kdelibs/plasma/private/packages.cpp If two files have the same key in Plasma::Package::addFileDefinition then only the first one will have a description, but if their keys aren't the same then both of them can have a description. Modified they key for the svgz files, is a good solution to our problem?(of course in another review) Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs (updated) - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review21311 --- you mean they can't have different descriptions? yes, that's true, and no, that's how it should be. if they are the stored in the same key, they are the same entry as far as the Package is concerned. - Aaron J. Seigo On Nov. 1, 2012, 3:08 p.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Nov. 1, 2012, 3:08 p.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 5cb3ea6 plasmate/editors/editpage.cpp 7e82ff2 plasmate/packagemodel.cpp 9eb0914 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/#review21135
---
plasmate/editors/editpage.cpp
http://git.reviewboard.kde.org/r/106680/#comment16556
I think that we need the entire
[plasmate]/themeImageDialog/$foo because
we do if (mimetype == i.key()) { ... }.
No?
- Giorgos Tsiapaliokas
On Oct. 4, 2012, 10:49 a.m., Giorgos Tsiapaliokas wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/
---
(Updated Oct. 4, 2012, 10:49 a.m.)
Review request for Plasma.
Description
---
create a new theme package-click on the new
a file dialog should appear but instead a simple edit box appears requesting
a new filename.
This patch solves the issue
Diffs
-
plasmate/editors/editpage.h 8cc5fab
plasmate/editors/editpage.cpp 91f6bce
plasmate/packagemodel.cpp 67cc0f4
Diff: http://git.reviewboard.kde.org/r/106680/diff/
Testing
---
Thanks,
Giorgos Tsiapaliokas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/#review21141
---
plasmate/editors/editpage.cpp
http://git.reviewboard.kde.org/r/106680/#comment16557
yes, so it should use startsWith :)
the code in the current patch is functionally equivalent to:
const QString themeType = [plasmate]/themeImageDialog/;
if (mimetype.startsWith(themeType)) {
const QString opts = cotents/ + mimetype.right(mimetype.size() -
themeType.length());
imageDialog(commonFilter, opts);
}
- Aaron J. Seigo
On Oct. 4, 2012, 10:49 a.m., Giorgos Tsiapaliokas wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106680/
---
(Updated Oct. 4, 2012, 10:49 a.m.)
Review request for Plasma.
Description
---
create a new theme package-click on the new
a file dialog should appear but instead a simple edit box appears requesting
a new filename.
This patch solves the issue
Diffs
-
plasmate/editors/editpage.h 8cc5fab
plasmate/editors/editpage.cpp 91f6bce
plasmate/packagemodel.cpp 67cc0f4
Diff: http://git.reviewboard.kde.org/r/106680/diff/
Testing
---
Thanks,
Giorgos Tsiapaliokas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: load the images for the themes correctly
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/#review20680 --- plasmate/packagemodel.cpp http://git.reviewboard.kde.org/r/106680/#comment16331 couldn't it just check for the string starting with [plasmate]/themeImageDialog instead of harcoding all these various values? would seem to be a lot more future proof. - Aaron J. Seigo On Oct. 4, 2012, 10:49 a.m., Giorgos Tsiapaliokas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106680/ --- (Updated Oct. 4, 2012, 10:49 a.m.) Review request for Plasma. Description --- create a new theme package-click on the new a file dialog should appear but instead a simple edit box appears requesting a new filename. This patch solves the issue Diffs - plasmate/editors/editpage.h 8cc5fab plasmate/editors/editpage.cpp 91f6bce plasmate/packagemodel.cpp 67cc0f4 Diff: http://git.reviewboard.kde.org/r/106680/diff/ Testing --- Thanks, Giorgos Tsiapaliokas ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
