Skip to content

CollectionInterface: add JSON error handling to avoid plugin crash #14

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.

Sign up for GitHub

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

Merged
pryrt merged 2 commits into from
Jun 14, 2025
Merged

CollectionInterface: add JSON error handling to avoid plugin crash #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1be52b6
CollectionInterface: add JSON error handling to avoid plugin crash
pryrt Jun 13, 2025
b3398bc
CollectionInterface: improve error handling to prevent crashes
pryrt Jun 13, 2025
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
232 changes: 149 additions & 83 deletions src/Classes/CollectionInterfaceClass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
CollectionInterface::CollectionInterface(HWND hwndNpp) {
_hwndNPP = hwndNpp;
_populateNppDirs();
getListsFromJson();
_areListsPopulated = getListsFromJson();
};

void CollectionInterface::_populateNppDirs(void) {
Expand Down Expand Up @@ -228,8 +228,9 @@ std::string CollectionInterface::_xml_unentity(const std::string& text)
}
#pragma warning ( pop )

void CollectionInterface::getListsFromJson(void)
bool CollectionInterface::getListsFromJson(void)
{
bool didThemeFail = false;
auto string2wstring = [](std::string str) {
if (str.empty()) return std::wstring();
int wsz = MultiByteToWideChar(CP_UTF8, 0, str.c_str(), static_cast<int>(str.size()), NULL, 0);
Expand All @@ -242,104 +243,169 @@ void CollectionInterface::getListsFromJson(void)
// Process Theme JSON
////////////////////////////////
std::vector<char> vcThemeJSON = downloadFileInMemory(L"https://raw.githubusercontent.com/notepad-plus-plus/nppThemes/master/themes/.toc.json");
nlohmann::json jTheme = nlohmann::json::parse(vcThemeJSON);
std::string v = jTheme.at(0).get<std::string>();
for (const auto& item : jTheme.items()) {
std::wstring ws = string2wstring(item.value().get<std::string>());
vThemeFiles.push_back(ws.c_str());
if (vcThemeJSON.empty()) {
// issue#13: do not continue if there's internet/connection problems
return false; // nothing downloaded, so want to know to close the download-dialog to avoid annoying user with useless empty listbox
}
else if (vcThemeJSON[0] != L'[') {
// related to issue#13: if downloadFileInMemory returns "404 Not Found" or similar, don't try to parse as JSON.
// easiest check: if the JSON isn't the expected [...] JSON array, don't continue with the _theme_;
// however, can still move to the UDL section, because that might still work, and since UDL is the primary purpose, it's probably worth it if UDL is working even if Themes aren't.
std::string msg = "Cannot interpret Themes Collection information:\n\n";
msg += vcThemeJSON.data();
if (msg.size() > 100) {
msg.resize(100);
msg += "\n...";
}
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: Download Problems", MB_ICONWARNING);
didThemeFail = true;
}
else {
Copy link
Copy Markdown

@ozone10 ozone10 Jun 13, 2025

Choose a reason for hiding this comment

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

I would use only if (!vcThemeJSON.empty()) and remove current if block,.
I don't expect .toc.json file content to be wrong.
In

NppPlugin-CollectionInterface/src/Classes/CollectionInterfaceClass.cpp

Line 57 in 374be61

std::vector<char> CollectionInterface::downloadFileInMemory(const std::wstring& url)

there are already message boxes with error description, and here seems to be redundant, And it can be annoying when you have to dismiss 4 message boxes and in the end get empty listbox.

Copy link
Copy Markdown
Owner Author

@pryrt pryrt Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would use only if (!vcThemeJSON.empty())

This would not catch the problem of GitHub sending a 404 or server error, because the vector will actually be the string of the error code (like 404 Not Found)

Copy link
Copy Markdown

@ozone10 ozone10 Jun 13, 2025

Choose a reason for hiding this comment

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

This would not catch the problem of GitHub not sending you any data: when there is a download error, the vector is not empty, it gives the HTTP error code (like 404 Not Found)

I see in that case you can add check for empty vector, to reduce message boxes.

In what situation does it give four message boxes? I can see that it might give 2 -- one for themes and one for UDL -- but I cannot see how it could possibly give 4.

image
image
image
image

I have a firewall configured to block applications the first time they try to connect to net, so I can to decide whether to allow them.

Copy link
Copy Markdown
Owner Author

@pryrt pryrt Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

And it can be annoying when you have to dismiss 4 message boxes and in the end get empty listbox.

It took me a while to figure out how there could be 4 message boxes. But I'm assuming now that your reported "connection is blocked" was actually one of the "Could not connect to internet when trying to download" message boxes that the plugin launches from inside the downloadFileInMemory() function.

Now that I think I've figured that out, I can figure out a good way to non-annoyingly handle that condition -- while still preventing other possible problems, like a 404 or 500 error from the GH server, or receiving junk rather than JSON for unknown reasons. (Because there are multiple types of errors, there will still be the extra indentation -- but since it's my code, I will do the blocks the best way for me to understand for future maintenance)

Copy link
Copy Markdown

@ozone10 ozone10 Jun 13, 2025

Choose a reason for hiding this comment

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

Another issue when clicking on "Download" button with empty listbox notepad++ will crash.

try
{
nlohmann::json jTheme = nlohmann::json::parse(vcThemeJSON);
std::string v = jTheme.at(0).get<std::string>();
for (const auto& item : jTheme.items()) {
std::wstring ws = string2wstring(item.value().get<std::string>());
vThemeFiles.push_back(ws.c_str());
}
}
catch (nlohmann::json::exception& e) {
std::string msg = std::string("JSON Error in Theme data: ") + e.what();
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: JSON Error", MB_ICONERROR);
didThemeFail = true;
}
catch (std::exception& e) {
std::string msg = std::string("Unrecognized Error in Theme data: ") + e.what();
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: Unrecognized Error", MB_ICONERROR);
didThemeFail = true;
}
}

////////////////////////////////
// Process UDL JSON
////////////////////////////////
std::vector<char> vcUdlJSON = downloadFileInMemory(L"https://raw.githubusercontent.com/notepad-plus-plus/userDefinedLanguages/refs/heads/master/udl-list.json");
nlohmann::json jUdl = nlohmann::json::parse(vcUdlJSON);
// for a list, the key() is just the index, and the value() is the sub-object
for (const auto& item : jUdl["UDLs"].items()) {
auto j = item.value();
std::wstring ws_id_name = string2wstring(j["id-name"].get<std::string>());
std::wstring udl_base = L"https://raw.githubusercontent.com/notepad-plus-plus/userDefinedLanguages/master/";

// Logic for UDL -> URL
if (j.contains("repository")) {
std::wstring sUDL = L"";
if (j["repository"].is_boolean()) { // URL repo should never be boolean; but if it is, generate default URL
sUDL = udl_base + L"UDLs/" + ws_id_name + L".xml";
}
if (j["repository"].is_string()) {
std::wstring ws = string2wstring(j["repository"].get<std::string>());
if (ws == L"") {
sUDL = udl_base + L"UDLs/" + ws_id_name + L".xml";
}
else if (ws.find(L"http") == 0) { // if string _starts_ with http or https, it's the full URL
sUDL = ws;
}
}

// assign into the data structure...
if (sUDL != L"") {
mapUDL[ws_id_name] = sUDL;
}
if (vcUdlJSON.empty()) {
// issue#13: do not continue if there's internet/connection problems
return false; // nothing downloaded, so want to know to close the download-dialog to avoid annoying user with useless empty listbox
}
else if (vcUdlJSON[0] != L'{') {
// related to issue#13: if downloadFileInMemory returns "404 Not Found" or similar, don't try to parse as JSON.
// easiest check: if the JSON isn't the expected [...] JSON array, don't continue with the _theme_;
std::string msg = "Cannot interpret UDL Collection information:\n\n";
msg += vcUdlJSON.data();
if (msg.size() > 100) {
msg.resize(100);
msg += "\n...";
}
if (!didThemeFail)
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: Download Problems", MB_ICONWARNING);
return false; // without UDL info, it's not worth displaying the Download Dialog
}
else {
Copy link
Copy Markdown

@ozone10 ozone10 Jun 13, 2025

Choose a reason for hiding this comment

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

Same as with vcThemeJSON but instead I would use

if (vcUdlJSON.empty())
{
    return;
}

to return early and reduce indentation level for next block for better readability.

try
{
nlohmann::json jUdl = nlohmann::json::parse(vcUdlJSON);
// for a list, the key() is just the index, and the value() is the sub-object
for (const auto& item : jUdl["UDLs"].items()) {
auto j = item.value();
std::wstring ws_id_name = string2wstring(j["id-name"].get<std::string>());
std::wstring udl_base = L"https://raw.githubusercontent.com/notepad-plus-plus/userDefinedLanguages/master/";

// Logic for UDL -> URL
if (j.contains("repository")) {
std::wstring sUDL = L"";
if (j["repository"].is_boolean()) { // URL repo should never be boolean; but if it is, generate default URL
sUDL = udl_base + L"UDLs/" + ws_id_name + L".xml";
}
if (j["repository"].is_string()) {
std::wstring ws = string2wstring(j["repository"].get<std::string>());
if (ws == L"") {
sUDL = udl_base + L"UDLs/" + ws_id_name + L".xml";
}
else if (ws.find(L"http") == 0) { // if string _starts_ with http or https, it's the full URL
sUDL = ws;
}
}

// Extract display-name
if (j.contains("display-name")) {
std::wstring wdisplay_name = string2wstring(_xml_unentity(j["display-name"].get<std::string>()));

// assign into the data structure...
if (wdisplay_name != L"") {
mapDISPLAY[ws_id_name] = wdisplay_name;
revDISPLAY[wdisplay_name] = ws_id_name;
}
}
// assign into the data structure...
if (sUDL != L"") {
mapUDL[ws_id_name] = sUDL;
}
}

// Logic for functionList -> URL
if (j.contains("functionList")) {
std::wstring wsFuncList = L"";
if (j["functionList"].is_boolean() && j["functionList"].get<bool>()) {
wsFuncList = udl_base + L"functionList/" + ws_id_name + L".xml";
}
if (j["functionList"].is_string()) {
std::wstring ws = string2wstring(j["functionList"].get<std::string>());
// Extract display-name
if (j.contains("display-name")) {
std::wstring wdisplay_name = string2wstring(_xml_unentity(j["display-name"].get<std::string>()));

if (ws.find(L"http") == 0) { // if string _starts_ with http or https, it's the full URL
wsFuncList = ws;
}
else {
wsFuncList = udl_base + L"functionList/" + ws + L".xml";
// assign into the data structure...
if (wdisplay_name != L"") {
mapDISPLAY[ws_id_name] = wdisplay_name;
revDISPLAY[wdisplay_name] = ws_id_name;
}
}
}

// assign wsFuncList into the data structure...
if (wsFuncList != L"") {
mapFL[ws_id_name] = wsFuncList;
}
}
// Logic for functionList -> URL
if (j.contains("functionList")) {
std::wstring wsFuncList = L"";
if (j["functionList"].is_boolean() && j["functionList"].get<bool>()) {
wsFuncList = udl_base + L"functionList/" + ws_id_name + L".xml";
}
if (j["functionList"].is_string()) {
std::wstring ws = string2wstring(j["functionList"].get<std::string>());

if (ws.find(L"http") == 0) { // if string _starts_ with http or https, it's the full URL
wsFuncList = ws;
}
else {
wsFuncList = udl_base + L"functionList/" + ws + L".xml";
}
}

// Logic for autoCompletion -> URL
if (j.contains("autoCompletion")) {
std::wstring wsAutoComp = L"";
if (j["autoCompletion"].is_boolean()) {
wsAutoComp = udl_base + L"autoCompletion/" + ws_id_name + L".xml";
}
if (j["autoCompletion"].is_string()) {
std::wstring ws = string2wstring(j["autoCompletion"].get<std::string>());
if (ws.find(L"http") == 0) {
wsAutoComp = ws;
}
else {
wsAutoComp = udl_base + L"autoCompletion/" + ws + L".xml";
// assign wsFuncList into the data structure...
if (wsFuncList != L"") {
mapFL[ws_id_name] = wsFuncList;
}
}
}

// assign sAutoComp into the data structure...
if (wsAutoComp != L"") {
mapAC[ws_id_name] = wsAutoComp;
// Logic for autoCompletion -> URL
if (j.contains("autoCompletion")) {
std::wstring wsAutoComp = L"";
if (j["autoCompletion"].is_boolean()) {
wsAutoComp = udl_base + L"autoCompletion/" + ws_id_name + L".xml";
}
if (j["autoCompletion"].is_string()) {
std::wstring ws = string2wstring(j["autoCompletion"].get<std::string>());
if (ws.find(L"http") == 0) {
wsAutoComp = ws;
}
else {
wsAutoComp = udl_base + L"autoCompletion/" + ws + L".xml";
}
}

// assign sAutoComp into the data structure...
if (wsAutoComp != L"") {
mapAC[ws_id_name] = wsAutoComp;
}
}
}
}
catch (nlohmann::json::exception& e) {
std::string msg = std::string("JSON Error in UDL data: ") + e.what();
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: JSON Error", MB_ICONERROR);
return false; // without UDL info, it's not worth displaying the Download Dialog
}
catch (std::exception& e) {
std::string msg = std::string("Unrecognized Error in UDL data: ") + e.what();
::MessageBoxA(_hwndNPP, msg.c_str(), "CollectionInterface: Unrecognized Error", MB_ICONERROR);
return false; // without UDL info, it's not worth displaying the Download Dialog
}
}

return;
// if it makes it here, there is enough data to be worth displaying the dialog
return true;
}

std::wstring& CollectionInterface::_wsDeleteTrailingNulls(std::wstring& str)
Expand Down Expand Up @@ -371,7 +437,7 @@ bool CollectionInterface::_is_dir_writable(const std::wstring& path)
std::wstring CollectionInterface::getWritableTempDir(void)
{
// first try the system TEMP
std::wstring tempDir(MAX_PATH+1, L'\0');
std::wstring tempDir(MAX_PATH + 1, L'\0');
GetTempPath(MAX_PATH + 1, const_cast<LPWSTR>(tempDir.data()));
_wsDeleteTrailingNulls(tempDir);

Expand Down Expand Up @@ -418,5 +484,5 @@ bool CollectionInterface::ask_overwrite_if_exists(const std::wstring& path)
if (!PathFileExists(path.c_str())) return true; // if file doesn't exist, it's okay to "overwrite" nothing ;-)
std::wstring msg = L"The path\r\n" + path + L"\r\nalready exists. Should I overwrite it?";
int ans = ::MessageBox(_hwndNPP, msg.c_str(), L"Overwrite File?", MB_YESNO);
return ans==IDYES;
return ans == IDYES;
}
4 changes: 3 additions & 1 deletion src/Classes/CollectionInterfaceClass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class CollectionInterface {
//bool downloadFileToDisk(const std::wstring& url, const std::string& path);
//bool downloadFileToDisk(const std::string& url, const std::wstring& path);
bool downloadFileToDisk(const std::wstring& url, const std::wstring& path);
void getListsFromJson(void);
bool getListsFromJson(void);

// getter methods
std::wstring nppCfgDir(void) { return _nppCfgDir; };
Expand All @@ -46,6 +46,7 @@ class CollectionInterface {
bool isFunctionListDirWritable(void) { return _is_dir_writable(_nppCfgFunctionListDir); };
bool isAutoCompletionDirWritable(void) { return _is_dir_writable(_nppCfgAutoCompletionDir); };
bool isThemesDirWritable(void) { return _is_dir_writable(_nppCfgThemesDir); };
bool areListsPopulated(void) { return _areListsPopulated; };

// if the chosen directory isn't writable, need to be able to use a directory that _is_ writable
// as a TempDir, and then will need to use runas to copy from the TempDir to the real dir.
Expand All @@ -69,4 +70,5 @@ class CollectionInterface {
_nppCfgThemesDir;

HWND _hwndNPP;
bool _areListsPopulated;
};
47 changes: 34 additions & 13 deletions src/Dialogs/CollectionInterfaceDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,22 @@ INT_PTR CALLBACK ciDlgProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), L"READY");

pobjCI = new CollectionInterface(hParent);
//pobjCI->getListsFromJson();
if (!pobjCI->areListsPopulated()) {
EndDialog(hwndDlg, 0);
DestroyWindow(hwndDlg);
g_hwndCIDlg = nullptr;
delete pobjCI;
pobjCI = NULL;
return true;
}
_populate_file_cbx(hwndDlg, pobjCI->mapUDL, pobjCI->mapDISPLAY);

// Dark Mode Subclass and Theme: needs to go _after_ all the controls have been initialized
LRESULT nppVersion = ::SendMessage(nppData._nppHandle, NPPM_GETNPPVERSION, 1, 0); // HIWORD(nppVersion) = major version; LOWORD(nppVersion) = zero-padded minor (so 8|500 will come after 8|410)
LRESULT darkdialogVersion = MAKELONG(540, 8); // NPPM_GETDARKMODECOLORS requires 8.4.1 and NPPM_DARKMODESUBCLASSANDTHEME requires 8.5.4
LRESULT localsubclassVersion = MAKELONG(810, 8); // from 8.540 to 8.810 (at least), need to do local subclassing because of tab control
g_IsDarkMode = (bool)::SendMessage(nppData._nppHandle, NPPM_ISDARKMODEENABLED, 0, 0);
if (g_IsDarkMode && (nppVersion>=darkdialogVersion)) {
if (g_IsDarkMode && (nppVersion >= darkdialogVersion)) {
::SendMessage(nppData._nppHandle, NPPM_GETDARKMODECOLORS, sizeof(NppDarkMode::Colors), reinterpret_cast<LPARAM>(&myColors));
myBrushes.change(myColors);
myPens.change(myColors);
Expand Down Expand Up @@ -219,13 +226,16 @@ INT_PTR CALLBACK ciDlgProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam
{
std::wstring wsCategory = _get_tab_category_wstr(hwndDlg, IDC_CI_TABCTRL);
LRESULT selectedFileIndex = ::SendDlgItemMessage(hwndDlg, IDC_CI_COMBO_FILE, LB_GETCURSEL, 0, 0);
switch (selectedFileIndex) {
case CB_ERR:
::MessageBox(NULL, L"Could not understand FILE combobox; sorry", L"Download Error", MB_ICONERROR);
return true;
if (selectedFileIndex == CB_ERR) {
::MessageBox(NULL, L"Could not understand name selection; sorry", L"Download Error", MB_ICONERROR);
return true;
}

LRESULT needFileLen = ::SendDlgItemMessage(hwndDlg, IDC_CI_COMBO_FILE, LB_GETTEXTLEN, selectedFileIndex, 0);
if (needFileLen == LB_ERR) {
::MessageBox(NULL, L"Could not understand name selection; sorry", L"Download Error", MB_ICONERROR);
return true;
}
std::wstring wsFilename(needFileLen, 0);
::SendDlgItemMessage(hwndDlg, IDC_CI_COMBO_FILE, LB_GETTEXT, selectedFileIndex, reinterpret_cast<LPARAM>(wsFilename.data()));

Expand Down Expand Up @@ -334,10 +344,12 @@ INT_PTR CALLBACK ciDlgProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam
}

// update progress bar
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100 * count / total, 0);
wchar_t wcDLPCT[256];
swprintf_s(wcDLPCT, L"Downloading %d%%", 100 * count / total);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
if (didDownload) {
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100 * count / total, 0);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
}

// also download AC and FL, if applicable
std::vector<std::wstring> xtra = { L"AC", L"FL" };
Expand Down Expand Up @@ -375,15 +387,24 @@ INT_PTR CALLBACK ciDlgProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam
}
}
// update progress bar
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100 * count / total, 0);
swprintf_s(wcDLPCT, L"Downloading %d%%", 100 * count / total);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
if (didDownload) {
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100 * count / total, 0);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
}
}

// Final update of progress bar: 100%
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100, 0);
swprintf_s(wcDLPCT, L"Downloading %d%% [DONE]", 100);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
if (didDownload) {
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 100, 0);
swprintf_s(wcDLPCT, L"Downloading %d%% [DONE]", 100);
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
}
else {
::SendDlgItemMessage(hwndDlg, IDC_CI_PROGRESSBAR, PBM_SETPOS, 0, 0);
swprintf_s(wcDLPCT, L"Nothing to Download. [DONE]");
Edit_SetText(GetDlgItem(hwndDlg, IDC_CI_PROGRESSLBL), wcDLPCT);
}
}
return true;
case IDC_CI_HELPBTN:
Expand Down