Add ability to do leading spaces with ColumnEditor #13336

alankilborn · 2023-03-11T21:36:56Z

alankilborn · 2023-03-11T21:46:39Z

Second commit changes nbChiffre to a more readable nbDigits; if this is not wanted, easy to separate out.

alankilborn · 2023-03-11T21:48:07Z

Tested the PR code:

none               |  zeros              |  spaces
0   0   0   0      |  00  00  00  00000  |   0   0   0      0
1   1   1   1      |  01  01  01  00001  |   1   1   1      1
2   2   2   10     |  02  02  02  00010  |   2   2   2     10
3   3   3   11     |  03  03  03  00011  |   3   3   3     11
4   4   4   100    |  04  04  04  00100  |   4   4   4    100
5   5   5   101    |  05  05  05  00101  |   5   5   5    101
6   6   6   110    |  06  06  06  00110  |   6   6   6    110
7   7   7   111    |  07  07  07  00111  |   7   7   7    111
8   8   10  1000   |  08  08  10  01000  |   8   8  10   1000
9   9   11  1001   |  09  09  11  01001  |   9   9  11   1001
10  A   12  1010   |  10  0A  12  01010  |  10   A  12   1010
11  B   13  1011   |  11  0B  13  01011  |  11   B  13   1011
12  C   14  1100   |  12  0C  14  01100  |  12   C  14   1100
13  D   15  1101   |  13  0D  15  01101  |  13   D  15   1101
14  E   16  1110   |  14  0E  16  01110  |  14   E  16   1110
15  F   17  1111   |  15  0F  17  01111  |  15   F  17   1111
16  10  20  10000  |  16  10  20  10000  |  16  10  20  10000

Yaron10 · 2023-03-12T13:36:16Z

@alankilborn,

Very nice. 👍
Thank you for your work.

Just another interesting case (NOT caused by this PR):
If you select a virtual column and insert text via the column-editor, the post-operation-selection is wrong.

alankilborn · 2023-03-12T13:52:33Z

the post-operation-selection is wrong.

That was noticed, but since it wasn't related to the change, I didn't worry about it.
The code change is already VERY large, with large risk of rejection on that basis.
(I finished this some time ago, but held back, and wasn't going to submit at all, given the amount of change for such small gain. Then I decided that the risk was worth it, to potentially provide users with the improvement)

donho

Please follow the suggestions.

PowerEditor/installer/nativeLang/english.xml

PowerEditor/src/Parameters.h

PowerEditor/src/ScintillaComponent/ScintillaEditView.h

PowerEditor/src/ScintillaComponent/columnEditor_rc.h

PowerEditor/src/ScintillaComponent/ScintillaEditView.cpp

Yaron10 · 2023-03-18T20:03:35Z

@alankilborn,

If the last selected section is "Text", we have "number" in config.xml. And vice-versa.

notepad-plus-plus/PowerEditor/src/Parameters.cpp

Line 2497 in 5645ca1

if (lstrcmp(strVal, TEXT("text")) == 0)

notepad-plus-plus/PowerEditor/src/Parameters.cpp

Line 4095 in 5645ca1

    
           (columnEditorRootNode.ToElement())->SetAttribute(TEXT("choice"), _columnEditParam._mainChoice ? L"number" : L"text");

It seems the values should be swapped.
Should I open a new issue?

alankilborn · 2023-03-18T23:22:41Z

@Yaron10

If the last selected section is "Text", we have "number" in config.xml. And vice-versa.
It seems the values should be swapped.
Should I open a new issue?

I'd say a resounding YES!

Yaron10 · 2023-03-18T23:38:59Z

#13370.

alankilborn · 2023-03-19T12:07:12Z

@donho, if it builds (still building as I type this), then this PR is ready for re-review.

Yaron10 · 2023-03-19T14:52:22Z

PowerEditor/src/ScintillaComponent/columnEditor.cpp

@@ -57,7 +57,15 @@ intptr_t CALLBACK ColumnEditorDlg::run_dlgProc(UINT message, WPARAM wParam, LPAR
 			::SendDlgItemMessage(_hSelf, IDC_COL_LEADING_COMBO, CB_ADDSTRING, 0, reinterpret_cast<LPARAM>(TEXT("None")));
 			::SendDlgItemMessage(_hSelf, IDC_COL_LEADING_COMBO, CB_ADDSTRING, 0, reinterpret_cast<LPARAM>(TEXT("Zeros")));
 			::SendDlgItemMessage(_hSelf, IDC_COL_LEADING_COMBO, CB_ADDSTRING, 0, reinterpret_cast<LPARAM>(TEXT("Spaces")));
-			::SendMessage(::GetDlgItem(_hSelf, IDC_COL_LEADING_COMBO), CB_SETCURSEL, colEditParam._leadingChoice, 0);
+			WPARAM curSel = 0;


May I ask why this block is necessary?
Can't leadingChoice be int?

Don wanted the "leading choice" to be an enum (see Don's prior comments). My goal here was to fully uncouple things. Again, I can't read Don's mind as to how "far" to take such things, so I proceeded on the presumption of this type of decoupling being wanted. I really have no idea. If the future gets "sticky" for me on this PR, I will abandon it and close it; of course an outright rejection will do that just as well, also. :-)

Don wanted the "leading choice" to be an enum

I meant to ask why the enum couldn't be int. :)

Well, let's for Don's review.

Yea, the way I read it was Don specifically wanted enum.
But maybe his real intent was to just not have two bool. I do understand the desire to not have two bool; indeed I only went with 2 bool in the beginning in an effort to change as little as possible.
The high volume of changed lines in this makes me uncomfortable -- not at a C++ level, but at a "this is someone else's project" level.
I see others aren't afraid to change a lot of lines; I think a lot of changes are more acceptable if the feature is really wanted.
I'm truly sorry that I got involved in this one; my intent was to stay with smaller changes.

I'm glad you got involved in this one. - You did a great job. 👍
I'm sorry you didn't enjoy working on it.

My main question was why the switch (colEditParam._leadingChoice) block was necessary.

"Necessary" might be a strong word, here. :-)
We can do things in a "hacky" way, and sure, that has been done in the past.

The code (that I think) you are questioning was changed to fully decouple the integer values of the combobox selection indices and the "values" of the enum type.

With this, the code for this line:
enum leadingChoice : UCHAR { noneLeading, zeroLeading, spaceLeading };
could be changed to this without any runtime difference:
enum leadingChoice : UCHAR { noneLeading=27, zeroLeading=45, spaceLeading=106 };

(Probably the UCHAR should be eliminated; this is a carryover of my habit because I work on smallish embedded systems where every byte can matter.)

Don seemed to want an enum ; when he requested this change, I provided, in a better and more complete fashion than an earlier commit (earlier commits aren't really relevant when changes are requested and made).

@donho

Edit: oups, I misread your comment. Good job, it's what it needs.

Thank you, Don, encouragement to your contributors that they are doing the right thing is very helpful indeed!

@alankilborn,

Thank you for the explanation. 👍
You're a very good coder, and I was trying to understand and learn your approach.

@Yaron10
It's true that I (as a lazy dev) I would do the following without switch:

enum leadingChoice { noneLeading = 0, zeroLeading = 1, spaceLeading = 2}; ::SendMessage(::GetDlgItem(_hSelf, IDC_COL_LEADING_COMBO), CB_SETCURSEL, (WPARAM)colEditParam._leadingChoice, 0);

The @alankilborn 's way guarantees strongly the value must be "> 3" & "<= 0".
As I always check both side while crossing the one way road IRL, I can understand his approach - and since it's not redundant, it's OK to me.

@donho,

The "lazy approach" was what I had in mind.
Thank you for the detailed explanation. 👍

Yaron10 · 2023-03-24T18:22:13Z

@alankilborn,

If the Format is Hex, Initial Number is 999 and Increase is blank - you always get 4 characters.
If Initial Number is 99, you get 2 characters.

What do you think?

alankilborn · 2023-03-24T18:50:07Z

If the Format is Hex, Initial Number is 999 and Increase is blank - you always get 4 characters. If Initial Number is 99, you get 2 characters.

What do you think?

notepad-plus-plus/PowerEditor/src/ScintillaComponent/ScintillaEditView.cpp

Lines 178 to 179 in c59c07b

    
           if ((base == 16) && (nbChiffre % 2 != 0)) 
        
           	nbChiffre += 1;

If the hex number needs an odd number of digits for display, it is increased to the next even number of digits -- no idea why this would be this way; this PR isn't related.

Yaron10 · 2023-03-24T19:08:53Z

@alankilborn,

Yes, it's not related to this PR.
But it is related to #11148 (comment), and I thought that mini-discussion was held here.

Thank you.

alankilborn · 2023-03-24T19:54:13Z

But it is related to 11148, and I thought that mini-discussion was held here.

I reread that now and I'm so confused.
Is THAT relevant to current issue and PR?
If not, it needs to be a separate issue, to avoid confusion.

Yaron10 · 2023-03-24T20:06:44Z

You asked for my opinion in that thread, and I raised that point as a side-note.
Sorry for the confusion.

alankilborn · 2023-03-24T20:15:13Z

I raised that point as a side-note.

OK... I don't think the current issue and PR can/should be a catch-all to all wacky behavior with Column Editor. I think current issue and PR should address the basic deficiency (of not currently -- 8.5.1 -- being able to insert numbers with leading spaces) and that other behavior should be addressed separately. I suggest waiting until this PR is integrated/rejected and then making issues on further defects.

alankilborn added 2 commits March 11, 2023 16:35

Add ability to do leading spaces with ColumnEditor

60714f3

Add ability to do leading spaces with ColumnEditor

d8a46eb

donho self-assigned this Mar 12, 2023

donho requested changes Mar 13, 2023

View reviewed changes

Add ability to do leading spaces with ColumnEditor

193e593

Yaron10 reviewed Mar 19, 2023

View reviewed changes

donho added the accepted label Mar 21, 2023

donho approved these changes Mar 25, 2023

View reviewed changes

donho closed this in 7f54a4b Mar 25, 2023

alankilborn mentioned this pull request Mar 25, 2023

ColumnEditor hex number insertion always uses even number of digits #13413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to do leading spaces with ColumnEditor #13336

Add ability to do leading spaces with ColumnEditor #13336

alankilborn commented Mar 11, 2023 •

edited

Loading

alankilborn commented Mar 11, 2023

alankilborn commented Mar 11, 2023

Yaron10 commented Mar 12, 2023

alankilborn commented Mar 12, 2023

donho left a comment

Yaron10 commented Mar 18, 2023

alankilborn commented Mar 18, 2023

Yaron10 commented Mar 18, 2023

alankilborn commented Mar 19, 2023

Yaron10 Mar 19, 2023

alankilborn Mar 19, 2023

Yaron10 Mar 19, 2023

alankilborn Mar 19, 2023

Yaron10 Mar 19, 2023

alankilborn Mar 21, 2023

alankilborn Mar 21, 2023

Yaron10 Mar 21, 2023

donho Mar 21, 2023

Yaron10 Mar 21, 2023

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023 •

edited

Loading

Add ability to do leading spaces with ColumnEditor #13336

Add ability to do leading spaces with ColumnEditor #13336

Conversation

alankilborn commented Mar 11, 2023 • edited Loading

alankilborn commented Mar 11, 2023

alankilborn commented Mar 11, 2023

Yaron10 commented Mar 12, 2023

alankilborn commented Mar 12, 2023

donho left a comment

Choose a reason for hiding this comment

Yaron10 commented Mar 18, 2023

alankilborn commented Mar 18, 2023

Yaron10 commented Mar 18, 2023

alankilborn commented Mar 19, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023

Yaron10 commented Mar 24, 2023

alankilborn commented Mar 24, 2023 • edited Loading

alankilborn commented Mar 11, 2023 •

edited

Loading

alankilborn commented Mar 24, 2023 •

edited

Loading