Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions data/strings/en.ini
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ deleting_tilemaps_will_delete_tilesets = Warning\n<<Deleting the following tilem
cannot_file_overwrite_on_export = Overwrite Warning\n<<You cannot Export with the same name (overwrite the original file).\n<<Use "File > Save As" menu option in that case.\n||&OK
cannot_open_file = Problem<<Cannot open file:<<{0}||&OK
cannot_open_folder = Problem<<Cannot open folder:<<{0}||&OK
cannot_open_theme = Error<<The selected theme ID is "default". Please change<<the theme ID inside "package.json" to make it work.||&OK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this message needed? Because it looks like in the place where cannot_open_theme is used it should be cannot_install_default_extension instead (e.g. when we use "Add Extension" we could show cannot_install_default_extension).

cannot_save_in_read_only_file = Problem<<The selected file is read-only. Try with other file.||&Go back
clipboard_access_locked = Error<<Cannot access to the clipboard.<<Maybe other application is using it.||&OK
clipboard_image_format_not_supported = Error<<The current clipboard image format is not supported.||&OK
Expand Down Expand Up @@ -90,6 +91,7 @@ job_working = {0}<<Working...||&Cancel
nothing_to_report = Crash Report<<Nothing to report||&OK
install_extension = Warning\n<<Do you really want to install the given extension?\n<<"{0}"\n||&Install||&Cancel
uninstall_extension_warning = Warning\n<<Do you really want to uninstall "{0}" extension?\n||&Yes||&No
cannot_install_default_extension = Error\n<<This theme cannot be installed because its name matches the default aseprite theme\n||&OK
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the message to something like:

Suggested change
cannot_install_default_extension = Error\n<<This theme cannot be installed because its name matches the default aseprite theme\n||&OK
cannot_install_default_extension = Error<<This extension cannot be installed because tries to replace the default theme.<<<<Contact the developer so they fix the theme ID string from \"default\" to something else.||&OK

unknown_output_file_format_error = Aseprite\n<<Unknown file format "{0}" in output filename\n||&OK
update_screen_ui_scaling_with_theme_values = Update Screen/UI Scaling\n<<The new theme "{0}" wants to adjust some values for you:\n<< Screen Scaling: {1}% -> {2}%\n<< UI Scaling: {3}% -> {4}%\n<<Allow these changes?\n||&Adjust Scaling||&Don't Adjust Scaling
update_extension = Update Extension\n<<The extension "{0}" already exists.\n<<Do you want to {1} from v{2} to v{3}?\n||&Yes||&No
Expand Down
48 changes: 47 additions & 1 deletion src/app/commands/cmd_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,18 @@ class OptionsWindow : public app::gen::Options {
}
}

// Get the extension information from the compressed
// package.json file.
const ExtensionInfo info =
App::instance()->extensions().getCompressedExtensionInfo(filename);
// Check if the filename corresponds to aseprite-default theme
if (base::string_to_lower(info.name) ==
Extensions::kAsepriteDefaultThemeName) {
Comment on lines +934 to +935
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably ExtensionInfo needs a bool property like defaultTheme instead of the themeId (below more details).

ui::Alert::show(
fmt::format(Strings::alerts_cannot_install_default_extension()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for fmt::format():

Suggested change
fmt::format(Strings::alerts_cannot_install_default_extension()));
Strings::alerts_cannot_install_default_extension());

return false;
}

// Install?
if (ui::Alert::show(Strings::alerts_install_extension(filename)) != 1)
return false;
Expand Down Expand Up @@ -1378,7 +1390,8 @@ class OptionsWindow : public app::gen::Options {
if (!ext->isEnabled())
continue;

if (ext->themes().empty())
if (ext->themes().empty() ||
isExtensionADuplicatedDefaultTheme(ext))
continue;

if (first) {
Expand Down Expand Up @@ -1410,6 +1423,9 @@ class OptionsWindow : public app::gen::Options {
extensionsList()->addChild(sep);
for (auto e : App::instance()->extensions()) {
if (e->category() == category) {
if (category == Extension::Category::Themes &&
isExtensionADuplicatedDefaultTheme(e))
continue;
ExtensionItem* item = new ExtensionItem(e);
extensionsList()->addChild(item);
hasItems = true;
Expand Down Expand Up @@ -1571,6 +1587,11 @@ class OptionsWindow : public app::gen::Options {
// package.json file.
ExtensionInfo info = exts.getCompressedExtensionInfo(filename);

if (info.themeId == "default") {
ui::Alert::show(
fmt::format(Strings::alerts_cannot_open_theme()));
return;
}
// Check if the extension already exist
for (auto ext : exts) {
if (base::string_to_lower(ext->name()) !=
Expand Down Expand Up @@ -1749,6 +1770,17 @@ class OptionsWindow : public app::gen::Options {
return paths;
}

static base::paths getUserDirPaths(const base::paths& dirNames) {
ResourceFinder rf;
for (auto& fn : dirNames)
rf.includeUserDir(fn.c_str());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

base::paths paths;
while (rf.next())
paths.push_back(base::normalize_path(rf.filename()));
return paths;
}

void updateCategoryVisibility() {
bool visibleCategories[int(Extension::Category::Max)];
for (auto& v : visibleCategories)
Expand All @@ -1764,6 +1796,20 @@ class OptionsWindow : public app::gen::Options {
}
}

// Function to determine if the input extension is the default theme
static bool isExtensionADuplicatedDefaultTheme(const Extension* e) {
if (!e->isDefaultTheme())
return false;
auto userThemePaths =
getUserDirPaths({"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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
return false;
}

#ifdef LAF_WINDOWS
void onTabletAPIChange() {
const bool pointerApi = tabletApiWindowsPointer()->isSelected();
Expand Down
14 changes: 11 additions & 3 deletions src/app/extensions.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2020-2023 Igara Studio S.A.
// Copyright (C) 2020-2024 Igara Studio S.A.
// Copyright (C) 2017-2018 David Capello
//
// This program is distributed under the terms of
Expand Down Expand Up @@ -51,12 +51,13 @@

namespace app {

const char* Extensions::kAsepriteDefaultThemeName = "aseprite-theme";

Comment on lines +54 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need of this change if the ExtensionInfo includes a bool to know if it overrides the default theme.

namespace {

const char* kPackageJson = "package.json";
const char* kInfoJson = "__info.json";
const char* kPrefLua = "__pref.lua";
const char* kAsepriteDefaultThemeExtensionName = "aseprite-theme";

class ReadArchive {
public:
Expand Down Expand Up @@ -283,6 +284,8 @@ void Extension::addTheme(const std::string& id,
const std::string& path,
const std::string& variant)
{
if (id == "default" && !isDefaultTheme())
return;
Comment on lines +287 to +288
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some comment and extra constant (should we add an ASSERT() too?):

Suggested change
if (id == "default" && !isDefaultTheme())
return;
// Avoid adding a theme with "default" ID if it's not
// the default aseprite-theme extension.
if (id == kAsepriteDefaultThemeName && !isDefaultTheme())
return;

So we have kAsepriteDefaultThemeExtensionName = "aseprite-theme" and a new kAsepriteDefaultThemeName = "default".

m_themes[id] = ThemeInfo(path, variant);
updateCategory(Category::Themes);
}
Expand Down Expand Up @@ -519,7 +522,7 @@ bool Extension::isCurrentTheme() const

bool Extension::isDefaultTheme() const
{
return (name() == kAsepriteDefaultThemeExtensionName);
return name() == Extensions::kAsepriteDefaultThemeName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback this change.

}

void Extension::updateCategory(const Category newCategory)
Expand Down Expand Up @@ -1003,6 +1006,11 @@ ExtensionInfo Extensions::getCompressedExtensionInfo(const std::string& zipFn)
std::string err;
auto json = json11::Json::parse(out.str(), err);
if (err.empty()) {
if (json["contributes"].is_object()) {
auto themes = json["contributes"]["themes"];
if (themes.is_array() && themes[0].is_object())
info.themeId = themes[0]["id"].string_value();
Comment on lines +1011 to +1012
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check all themes, one extension can have multiple items in the "themes" array (and the second one could be called "default"):

Suggested change
if (themes.is_array() && themes[0].is_object())
info.themeId = themes[0]["id"].string_value();
if (themes.is_array()) {
for (...) {
if (themes[i]["id"].string_value() == kAsepriteDefaultThemeName) {
info.defaultTheme = true;
break;
}
}
}

}
info.name = json["name"].string_value();
info.version = json["version"].string_value();
info.dstPath = base::join_path(m_userExtensionsPath, info.name);
Expand Down
7 changes: 5 additions & 2 deletions src/app/extensions.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Aseprite
// Copyright (C) 2020-2023 Igara Studio S.A.
// Copyright (C) 2020-2024 Igara Studio S.A.
// Copyright (C) 2017-2018 David Capello
//
// This program is distributed under the terms of
Expand Down Expand Up @@ -30,6 +30,7 @@ namespace app {
class Extensions;

struct ExtensionInfo {
std::string themeId;
std::string name;
std::string version;
std::string dstPath;
Comment on lines +33 to 36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a bool just to indicate that this extension info contains one theme that tries to overwrite the default one:

Suggested change
std::string themeId;
std::string name;
std::string version;
std::string dstPath;
std::string name;
std::string version;
std::string dstPath;
bool defaultTheme = false;

Expand All @@ -54,6 +55,8 @@ namespace app {
Max
};

bool isDefaultTheme() const;

Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shouldn't be needed with the new field in ExtensionInfo.

class DitheringMatrixInfo {
public:
DitheringMatrixInfo();
Expand Down Expand Up @@ -150,7 +153,6 @@ namespace app {
void uninstall(const DeletePluginPref delPref);
void uninstallFiles(const std::string& path,
const DeletePluginPref delPref);
bool isDefaultTheme() const;
void updateCategory(const Category newCategory);
#ifdef ENABLE_SCRIPTING
void initScripts();
Expand Down Expand Up @@ -196,6 +198,7 @@ namespace app {
public:
typedef std::vector<Extension*> List;
typedef List::iterator iterator;
static const char* kAsepriteDefaultThemeName;

Extensions();
~Extensions();
Expand Down
Loading