Fix Ignore any third-party theme that uses "default" as ID (fix #4226) #4335

Gasparoken · 2024-02-23T21:24:20Z

Pending alert artist when a theme with default ID is omitted.

fix #4226

aseprite-bot · 2024-03-01T20:34:37Z

clang-tidy review says "All clean, LGTM! 👍"

Gasparoken · 2024-03-01T20:44:31Z

Hello @martincapello. This solution hides any "aseprite-theme" themes from the Extensions and Theme lists.

martincapello · 2024-03-19T11:30:34Z

src/app/commands/cmd_options.cpp

@@ -1409,6 +1410,8 @@ class OptionsWindow : public app::gen::Options {
 extensionsList()->addChild(sep);
 for (auto e : App::instance()->extensions()) {
 if (e->category() == category) {
+ if (isExtensionADuplicatedDefaultTheme(e))


Should we check that this if is executed only for the Extension::Category::Themes category?
I know that it a case that it is not likely to happen...but mentioning just in case.

martincapello · 2024-03-19T11:35:42Z

src/app/commands/cmd_options.cpp

@@ -1741,9 +1744,10 @@ class OptionsWindow : public app::gen::Options {
 return base::normalize_path(rf.defaultFilename());
 }

- static base::paths themeFolders() {
+ static base::paths themeFolders(const std::vector<std::string> filenames) {


I think I would rename thefilenames parameter by dirnames

There is base::paths for this kind of type (list of paths).

martincapello · 2024-03-19T11:52:14Z

src/app/commands/cmd_options.cpp

 ResourceFinder rf;
- rf.includeDataDir(skin::SkinTheme::kThemesFolderName);
+ for (auto& fn : filenames)
+ rf.includeUserDir(fn.c_str());



If feels like some logic was lost here. Because before this change rf.includeDataDir was called, now we are calling rf.includeUserDir, and as far as I see they include different paths. Although I couldn't find a case where this behaves incorrectly when testing...so not sure about this.

martincapello · 2024-03-19T18:00:39Z

src/app/commands/cmd_options.cpp

+ themeFolders({"extensions", skin::SkinTheme::kThemesFolderName});
+ for (auto& p : userThemePaths) {
+ // Has the user path (p) the same path of the extension (e->path())?
+ if (std::strncmp(e->path().c_str(), p.c_str(), p.size()) == 0)


Since e->path() and p are std::strings, this could be replaced by:

if (e->path() == p)

Unfortunately that expression does not work, nor does the following one:

if (e->path().compare(p) == 0)

I'll return with

if (std::strncmp(e->path().c_str(), p.c_str(), p.size()) == 0)

Sorry, you are right, I didn't see that you where comparing up to p.size() characters and not the full strings.

In this case, the right one would be:

if ( e->path().compare(0, p.size(), p) == 0)

But I don't think it makes any difference...so I'm fine if you leave the std::strncmp() call as is.

…ite#4226) This fix hides user themes whose name is the same as the default, that is, "aseprite-theme" and are present in the user folders (i.e. 'extensions' and 'data/themes' folders).

aseprite-bot · 2024-04-10T20:47:18Z

clang-tidy review says "All clean, LGTM! 👍"

dacap · 2024-04-15T15:46:44Z

src/app/commands/cmd_options.cpp

@@ -1756,6 +1760,17 @@ class OptionsWindow : public app::gen::Options {
 return paths;
 }

+ static base::paths createUserDirPaths(const base::paths dirNames) {


I'd change the name of this function to something like

Suggested change

static base::paths createUserDirPaths(const base::paths dirNames) {

static base::paths getUserDirPaths(const base::paths dirNames) {

because when I saw it for first time I thought it was creating (mkdir) the directories.

dacap · 2024-04-15T15:51:23Z

I've just tested with a simple dacap-theme.aseprite-extension (with the same content of the default aseprite-theme) and it allows me to add the extension:

After reopening the Preferences dialog the installed extensions is correctly not displayed anymore. But the extension keeps installed in the user directory (c:/Users/David/AppData/Roaming/Aseprite/extensions/aseprite-theme).

I think we should show an error message and not install/uncompress the extension in the user directory.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-04-16T15:51:14Z

src/app/commands/cmd_options.cpp

+ base::string_to_lower(info.name))
+ return true;
+ return false;
+ }


warning: method 'filenameMatchesDefaultTheme' can be made static [readability-convert-member-functions-to-static]

Suggested change

}

static bool filenameMatchesDefaultTheme(const std::string& filename) {

aseprite-bot · 2024-04-16T15:51:14Z

src/app/commands/cmd_options.cpp

+ catch (const std::exception& ex) {
+ Console::showException(ex);
+ }
+ }


warning: variable 'info' of type 'ExtensionInfo' can be declared 'const' [misc-const-correctness]

Suggested change

}

ExtensionInfo const info =

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2024-04-16T16:21:55Z

src/app/commands/cmd_options.cpp

@@ -940,6 +940,18 @@ class OptionsWindow : public app::gen::Options {
 }
 }

+ // Get the extension information from the compressed
+ // package.json file.
+ ExtensionInfo info =


warning: variable 'info' of type 'ExtensionInfo' can be declared 'const' [misc-const-correctness]

Suggested change

ExtensionInfo info =

ExtensionInfo const info =

…t aseprite-theme (fix aseprite#4226)

aseprite-bot · 2024-04-16T16:29:10Z

clang-tidy review says "All clean, LGTM! 👍"

Gasparoken · 2024-04-16T16:39:14Z

Ready for review.

Gasparoken assigned dacap Feb 23, 2024

Gasparoken requested a review from dacap as a code owner February 23, 2024 21:24

Gasparoken assigned Gasparoken and unassigned dacap Feb 26, 2024

Gasparoken force-pushed the fix-default-theme branch from 96b8ed0 to 7111683 Compare March 1, 2024 20:31

Gasparoken requested a review from martincapello March 1, 2024 20:44

Gasparoken assigned martincapello and unassigned Gasparoken Mar 1, 2024

martincapello reviewed Mar 19, 2024

View reviewed changes

martincapello assigned Gasparoken and unassigned martincapello Mar 19, 2024

Fix Ignore any third-party theme that uses "default" as ID (fix asepr…

5d49a03

…ite#4226) This fix hides user themes whose name is the same as the default, that is, "aseprite-theme" and are present in the user folders (i.e. 'extensions' and 'data/themes' folders).

Gasparoken force-pushed the fix-default-theme branch from 7111683 to 5d49a03 Compare April 10, 2024 20:44

Gasparoken assigned martincapello and unassigned Gasparoken Apr 10, 2024

martincapello approved these changes Apr 12, 2024

View reviewed changes

martincapello assigned Gasparoken and unassigned martincapello Apr 12, 2024

Gasparoken assigned dacap and unassigned Gasparoken Apr 12, 2024

dacap reviewed Apr 15, 2024

View reviewed changes

dacap assigned Gasparoken and unassigned dacap Apr 15, 2024

aseprite-bot reviewed Apr 16, 2024

View reviewed changes

Gasparoken force-pushed the fix-default-theme branch from 2adfd0b to af5b30a Compare April 16, 2024 16:18

aseprite-bot reviewed Apr 16, 2024

View reviewed changes

Fix do not allow to install theme with the same content of the defaul…

99de589

…t aseprite-theme (fix aseprite#4226)

Gasparoken force-pushed the fix-default-theme branch from af5b30a to 99de589 Compare April 16, 2024 16:25

Gasparoken assigned dacap and unassigned Gasparoken Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Ignore any third-party theme that uses "default" as ID (fix #4226) #4335

Fix Ignore any third-party theme that uses "default" as ID (fix #4226) #4335

Gasparoken commented Feb 23, 2024

aseprite-bot commented Mar 1, 2024

Gasparoken commented Mar 1, 2024

martincapello Mar 19, 2024

martincapello Mar 19, 2024

dacap Mar 19, 2024

martincapello Mar 19, 2024

martincapello Mar 19, 2024

Gasparoken Apr 10, 2024

martincapello Apr 12, 2024

aseprite-bot commented Apr 10, 2024

dacap Apr 15, 2024

dacap commented Apr 15, 2024

aseprite-bot left a comment

aseprite-bot Apr 16, 2024

aseprite-bot Apr 16, 2024

aseprite-bot left a comment

aseprite-bot Apr 16, 2024

aseprite-bot commented Apr 16, 2024

Gasparoken commented Apr 16, 2024

	static base::paths createUserDirPaths(const base::paths dirNames) {
	static base::paths getUserDirPaths(const base::paths dirNames) {

	}
	static bool filenameMatchesDefaultTheme(const std::string& filename) {

Fix Ignore any third-party theme that uses "default" as ID (fix #4226) #4335

Are you sure you want to change the base?

Fix Ignore any third-party theme that uses "default" as ID (fix #4226) #4335

Conversation

Gasparoken commented Feb 23, 2024

aseprite-bot commented Mar 1, 2024

Gasparoken commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot commented Apr 10, 2024

Choose a reason for hiding this comment

dacap commented Apr 15, 2024

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseprite-bot commented Apr 16, 2024

Gasparoken commented Apr 16, 2024