Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jul 31, 2018

Fixes #15, #98, #96, #78, #69

@ghost
Copy link
Author

ghost commented Aug 1, 2018

Please put on hold for the moment

// Menu item
if (0 == strcmp(element, "ITEM"))
{
element = strtok_s(NULL, "/", &context);
m_menuItems.push_back(tstring(WcharMbcsConverter::char2tchar(element).get()));
m_menuScripts.push_back(tstring(WcharMbcsConverter::char2tchar(element).get()));
if ((element[1] == ':') || (element[1] == '\\'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a check that item 1 of element exists and no null deref happens.

@chcg
Copy link
Collaborator

chcg commented Aug 1, 2018

@ClaudiaFrank I will wait for your go. See my code comment.

@chcg
Copy link
Collaborator

chcg commented Aug 3, 2018

Yes, just unicode builds are currently done and therefore #define UNICODE is set in VS project. So the part inside of #ifdef UNICODE is used. We don't need to break the ANSI build from the else part within this commit.
Think we could create an issue which deals with that and remove all the unicode ifdefs within one dedicated commit.

@ghost
Copy link
Author

ghost commented Aug 3, 2018

OK - will revert that part

::ShellExecuteA(m_hNotepad, "open", helpFile.c_str(), NULL, NULL, SW_SHOWDEFAULT);
helpFile = L"http://npppythonscript.sourceforge.net/docs/";
helpFile.append(WcharMbcsConverter::char2tchar(PYSCR_VERSION_STRING).get());
::ShellExecute(m_hNotepad, L"open", helpFile.c_str(), NULL, NULL, SW_SHOWNORMAL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

appending of the topic is not done here. Why?

Copy link
Author

Choose a reason for hiding this comment

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

This is part of the old (1.0.8) online help which can't be used anyway as it isn't setup for
versions > 1.0.8, therefore I haven't done anything.

Copy link
Collaborator

@chcg chcg Aug 31, 2018

Choose a reason for hiding this comment

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

@bruderstein Any plans to update the webpage at sourceforge? Or do you want to shift it to e.g. https://pages.github.com/?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ClaudiaFrank Could this PR be merged now? Or is there something open? Could you please extend the description what else is fixed apart from #15.

Copy link
Author

Choose a reason for hiding this comment

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

Despite of a missing online html help webpage, it should be ready to merge.
Description extended.

DWORD cchOUT = 0;
HRESULT res = ::AssocQueryString(ASSOCF_INIT_IGNOREUNKNOWN, ASSOCSTR_EXECUTABLE, L".html", NULL, pszOut, &cchOUT);
if (res = S_FALSE)
pszOut = new TCHAR[cchOUT];
Copy link
Collaborator

Choose a reason for hiding this comment

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

curly braces are missing for this section

Copy link
Author

Choose a reason for hiding this comment

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

This happens sometimes if a pythonista is trying to code in c++ ;-)
Will be fixed with next commit.

Copy link

Choose a reason for hiding this comment

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

If we're talking about (what appears to be) the single-line-30 after the IF in line 29 above, then C/C++ doesn't require curly braces...(but I always use them)...but maybe the P.S. coding standard does before code is accepted? Also, what's the standard for number of spaces for a "tab"...above I think I see 8 (which no one really uses!) which leads me to believe that maybe there is really an actual tab character there...and this would disappoint me if THAT is the coding standard...until Notepad++ fully implements a tab-indent-space-align feature. :-)

Copy link
Author

Choose a reason for hiding this comment

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

If I would have put in the braces in first place you wouldn't have been confused - it is a
complete code block (11 lines) and afaik PS is following npp coding guidelines.
My tab width is set to 4 spaces (no replacement by 4 spaces) - it seems github uses
a width of 8 spaces.

Copy link

Choose a reason for hiding this comment

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

I just used that to bring up wrapping single lines in braces...I like that..and a mention of what the coding standard for P.S. might be. Unfortunately, N++'s source is a real mess tab/spaces-wise, Tab characters always end up causing a source code mess in evolving projects with different contributors. Sorry to get this off the main track...Claudia, remember those "curlies" in the future! :-)

Copy link
Author

Choose a reason for hiding this comment

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

It would be nice if an extension exists which allows to set the coding guidelines per project.
I haven't found one so far but to be honest, didn't really try hard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Ah, no sorry misunderstanding I'm looking for something more advanced like
checking if the function, class, variable names are correct or if braces are in place etc...
Something which basically double checks the npp coding styles AND for VS2017.
I'm honest, I'm using Visual Studio :-) to code python script plugin.

@chcg chcg merged commit 1d9230f into bruderstein:master Sep 12, 2018
@chcg chcg added this to the v1.3 milestone Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants