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

Column Editor OK button should be disabled if Text-to-Insert is empty #13444

Conversation

doug1234
Copy link
Contributor

Fix #13315

@chcg chcg added the enhancement Proposed enhancements of existing features label Mar 30, 2023
Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Please make code simpler and straight forward.

if (toText)
{
ColumnEditorParam colEditParam = NppParameters::getInstance()._columnEditParam;
if (!colEditParam._insertedTextContent.empty())
Copy link
Member

Choose a reason for hiding this comment

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

It'll be more straight forward to get text from the text field control.

ColumnEditorParam colEditParam = NppParameters::getInstance()._columnEditParam;
if (!colEditParam._insertedTextContent.empty())
{
::SetDlgItemText(_hSelf, IDC_COL_TEXT_EDIT, colEditParam._insertedTextContent.c_str());
Copy link
Member

@donho donho Apr 2, 2023

Choose a reason for hiding this comment

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

Why do we need this line?
Or there's another bug?

}
else
{
::EnableWindow(::GetDlgItem(_hSelf, IDOK), true);
Copy link
Member

Choose a reason for hiding this comment

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

EnableWindow(::GetDlgItem(_hSelf, IDOK), xxx);
should be call only once at the end - xxx is the result of IDC_COL_TEXT_EDIT (empty or not).

@donho donho self-assigned this Apr 2, 2023
@doug1234
Copy link
Contributor Author

doug1234 commented Apr 4, 2023

I simplified the change based on your comments.

if (toText)
{
const int stringSize = 1024;
TCHAR str[stringSize];
Copy link
Contributor

Choose a reason for hiding this comment

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

const int stringSize = 1024; can be replaced with constexpr int stringSize = 1024;
Add initializer to be sure e.g. TCHAR str[stringSize]{};

Copy link
Member

Choose a reason for hiding this comment

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

@ozone10
good remark.
Could you add the "best practice" in contribution coding style:
https://github.com/notepad-plus-plus/notepad-plus-plus/blob/master/CONTRIBUTING.md#coding-style
Or create a issue for the "best practice" so I can add them in the guideline?

@donho
Copy link
Member

donho commented Apr 6, 2023

Life is complicated already, let's make our code simpler.
81db72a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed enhancements of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Column Editor OK button should be disabled if Text-to-Insert is empty
4 participants