ProgerXP
commented
7 years ago Additional notes regarding files in Extension.
- Check newly added operations (str2hex, etc.) if they can operate on rectangular selection. Most don't; for them add
IsSelectionModeValidwhich will produce a MsgBox. >Done. In additionIsSelectionModeValidbecame ton2e_ShowPromptIfSelectionModeIsRectangleto make the code clear. - Why some N2E_Trace are protected by _DEBUG but some are not? This needs to be uniformized; if you do limit tracing to debug builds then all trace calls should be protected. Perhaps better way is not to protect trace calls but trace functions (in Utils.c) themselves like it's done for trace macros. And move all logging/tracing to a separate .c/.h. >Done. Direct N2E_Trace()-calls were replaced with corresponding TRACE-macroses. Tracing subroutines changed to be completely "debug only" (as it was originally designed).
- Do you really need all
externs and inclusions in *.h or they were just copy pasted? >Finished with a cleanup for includes/extern references. - From what I understand, you do not prefix variables internal to a .c file with n2e_ or n2e. Then why some units have them all prefixed? >n2e-prefix is excess for the code isolated in separate 2E-modules (Extension folder). Originally, these names came from other developer(s) and now they were changed to comply common naming style.
- Was the large header comment in SciCall.c written by you? If yes then remove it since no other files have it. >It looks like SciCall.h came from another project notepad2-mod https://github.com/XhmikosR/notepad2-mod/commit/d6a6f3a6e1e33d4545223793388255d18aa81bf4#diff-4d4c7049a418e672be3d54e61d768b7f
EditHelper.c
Escape HTMLshould preserve selection unless original selection was empty. >Done- Is
elsemissing between}and{? If not then why a block is needed here?if (skipcl) { int ti = 0; for (; ti < skipcl; ++ti) { if (tchl - _left_braces == skipl[ti]) { N2E_TRACE("Skipped braces pair found '%c'", *tchl); skipl[ti] = -1; goto NEXT; } } } << { << N2E_TRACE("Left bracket found '%c'", lch); break; }>No, there is no missing
else-statement. Removed excess brackets.
ExtSelection.c
- "When one match one document" - what does it mean? > Only single word in the document matches selection.
- Explain "SE mode" in the comment.
>Rephrased comments for indicators:
//multiple words matches selection, some are located on other pages
define N2E_SELECT_INDICATOR 9
//single word matches
define N2E_SELECT_INDICATOR_SINGLE 10
//multiple words matches selection, all are visible on current page
define N2E_SELECT_INDICATOR_PAGE 11
// "selection edit"-mode, Ctrl+Tab to activate
define N2E_SELECT_INDICATOR_EDIT 12**
total count '- why apostrophe? > Count-postfix added to corresponding variable name, useless comment removed.- Is
icase_compareused anywhere? >Removed. - Why
to_loweruses hardcoded Russian char references? Why cannot it use a Unicode function? >Removed uselessto_lower. - Why Shift has special processing?
if (VK_RETURN == key && GetKeyState(VK_SHIFT) >= 0)>Shift-specific code was already removed during bug fixing for "new line"-commands.
- Why do you do this in
n2e_SelectionInit?iHighlightLineIfWindowInactive,iWordNavigationModedo not belong here.SendMessage(hwndEdit, SCI_SETCARETLINEVISIBLEALWAYS, iHighlightLineIfWindowInactive, 0); SendMessage(hwndEdit, SCI_SETWORDNAVIGATIONMODE, iWordNavigationMode, 0);>
n2e_SelectionInitrenamed ton2e_EditInitto change its sense. - Need to refactor 4 duplicated blocks in
n2e_SelectionInit. >Done. Added n2e_EditSelectionInit()-subroutine. - Correct English and/or meaning.
then we must don't check next matches Anyhow HL_SELECT_INDICATOR must be there ?!?!>Removed useless comments.
- Replace all
?!?!and???????with!. >Done.
StrToHex.c
Write unit tests that:
- take a text 512 KiB long with Unicode symbols from multiple ranges (i.e. not just English and Russian) >Done. Implemented support for small text samples and normal/big file samples.
- set document's encoding to one of UTF-8, UTF-16 LE, Windows-1251, Shift-JIS, Latin-1 (Windows-1250) >Done. Windows-1250/1251 actually does not affect the file contents encoding since these are only charsets.
- set document's line endings to cr, lf, cr/lf. >Actually, line ending types in the input samples does not relate to the text encoding testing task. We can provide file samples with different line endings but it looks like it has no sense because text encoding algorythms does operate on a binary level with char codes. And if test succeeded for cr/lf sample than it means both cr and lf chars encoded/decoded properly, test cases for cr- and lf- samples are useless.
- convert text to hex using the above settings, then convert result (hex) back to text and ensure that result is identical to the original. >Done. Implemented next testing scheme: 1. Load input sample text/file. 2. Encode sample using specified algorythm, check actual result matches pre-calculated result sample (expected result). Fail the test if these are different. 3. Decode result (result sample from previous step), check decoding result matches initial sample value. Fail the test if these are different. 4. Repeat steps 1-3 for all input samples. There is also an additional option which allow to pass working buffer size for tested algorythm to make sure it properly works when using different text ranges.
- convert text to hex using the above settings, add random line breaks, random spaces, change encoding and line endings to some other value (e.g. lf -> crlf), then convert result (hex) back to text and ensure that result is identical to the original. >Unfortunately, using of specified random parameters which affects the re-coding result violates different principals of unit testing best practices. 1. Text en/decoding test should test text en/decoding. There should be nothing(as possible) except text encoding calls. 2. Test should be self-sufficient(as much as possible), no dependencies, etc. 3. Using of dynamic data generation and/or text re-coding as a part of test overloads it and may cause issues related to these foreign subsystems(data generation, text re-coding). Complex test with multiple dependecies is hard to diagnose and support. However, I've added hardcoded hex samples for decoding which uses specified feature: skip spaces and line breaks when decoding.
Write same tests for upcoming Base64 (#122) and QP (#124). >Done.
Utils.c
- What is
n2e_SelEditTimerProcfor? >Already removed - Replace magic strings from the previous developer with #constants.
>Done.
1, In
n2e_EnumProc, instead of searching window title for substring (!!!) check its class as inEnumWndProc. >Done. Notepad2e windows are searched by window class prefix since different execution types (e.g. elevated, pasteboarded) affects the classname postfix. - Rename
HWM_RELOAD_SETTINGStoWM_N2E_RELOAD_SETTINGS. >Done. - Change value of
HWM_RELOAD_SETTINGStoWM_USER + 0xFFand ofN2E_WHEEL_TIMER_IDto0xFF. >Done. - Shouldn't
isspacebeiswspaceinstead (same with strchr/wcschr, etc.)? >Fixed. Additionally,n2e_IsSpaceis defined but used in just 1 place; calls to common functions like these should be normalized. >Done. - In
n2e_OFNHookProc, why return/break are positioned so weirdly? >Fixed. - Why
NULL != StrChr(L"_", ch)instead of simple comparison? >Done. - Notepad2.c has more than 10 identical long calls to
SetWindowTitle; they need to be moved into a short function that will collect arguments itself. This will also remove the need for ugly SaveWindowTitleParams and UpdateWindowTitle. >Done. n2e_UpdateWindowTitle()-calls are used everywhere.
cshnik
/// End of Foo.c \\\,// End of Foo.c).>Done
printtime.batused anywhere? >The bat-file is used to generate _version.h which contains build time shown in the About dialog.EditHelper.hand elsewhere. >Done. The point here was to rename the functions which are directly called from the N2-code.EditHelperEx.cpp? >Yes. There are just few cases where C++ code is urgently needed: COM for showing system context menu, regex-validation function. Some other functions may be added in order to avoid duplication of code.ExtSelection.chave 50+ space characters? >Donenn2e_SelectionNotificationHandler- "nn"? >DoneDialogs.c
Why does
_version.hcontain a date of12-May-16 10:54:02.01? The UI shows correct date. Is it unused? >printtime.batfile is automatically executed when building the project to generate _version.h which contains actual build time shown in the About dialog._version.hfile removed from the sources.It seems
IDC_EMAILwas renamed toIDC_MAIL(AboutDlgProc) - if so then why? >DoneThere were
LVS_EX_FULLROWSELECTcommented out inFavoritesDlgProc,FileMRUDlgProc,SelectEncodingDlgProc- bring it back, I want to try them. >DoneDo you know what
SetExplorerTheme(GetDlgItem(hwnd,IDC_FILEMRU));was supposed to do? > There was some kind of Explorer-like visual customization. Here are some details on it: https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/1e861d81-c15e-4453-a907-ddce96e8b313/using-setwindowtheme-to-customize-the-look-of-an-application-explorer-theme-is-widely-discussed?forum=windowsuidevelopmentWhat this commented out block was supposed to do?
>It looks like MRU's list control was originally developed as a virtual one with providing of the item's icons using this code. Absolutely the same code is now used in FileMRUIconThread().
Why
n2e_OpenMRULastis inDialogs.c, not inside Extended\? It doesn't seem to use any internal fields. >DoneEdit.c
Do you know why encodings such as JIS, EUC, GC2312 were commented out and removed? >N/A
Can you remind me why do we need new
adjustNewLinesparameter forEditGetClipboardText? >RemovedadjustNewLinesparameter during code cleanup. Retrieving of the clipboard text without adjustments is no longer required.You haven't removed one
WARN()call (you have removed others). >Doneremove_charandFindEditWndProcshould be in 2e-specific helpers? >DoneMost of
IDACC_BACKhandling (between Dlg functions) can and should be moved into a 2e function. >DoneMost of
WM_COMMANDhandling inEditInsertTagDlgProcshould be moved into a 2e function. >DoneWhy would Flo need
SciThemedWndProcand other removed functions at the end of Edit.c? >N/A. There was some kind of Explorer-like visual customization. Here are some details on it: https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/1e861d81-c15e-4453-a907-ddce96e8b313/using-setwindowtheme-to-customize-the-look-of-an-application-explorer-theme-is-widely-discussed?forum=windowsuidevelopmentInsert short info comments of form
// [2e](or/*likewise) before changes from the original code. It should tell the feature's name (e.g.Edit > Go to last change) and/or description of the code change/purpose and/or the #issue number (e.g. for directory lock issue). No need to insert comments before: commented out blocks that you have removed, non-functional changes (NULL->L"", replacing a call with an equivalent 2e function, 'if' -> '?:', etc.), code that was replaced with a single call to a function starting withn2e_orN2E_(this is because the modification is clear), new declarations in*.hand #inclusions in h/c that start withn2e_prefix, minor edits (e.g. added parameter(s) to a modified function). If more than one line was changed, then the block should be closed off with[/2e]comment. To determine changed blocks you can use my normalized sources. >Donemay be i need todoesn't read like proper English. >RemovedNotepad2.c
OnPaneSizeClick: is it possible to get rid of the timer and use right-click instead of double-click for copy-to-clipboard? >Donen2e_MsgCreatewhich is almost identical to the original make a new function withinNotepad2.clike you did withFileLoad<->_FileLoadso it doesn't break the diff. >DoneGetCurrentShowTitleMenuIDandGetCurrentLanguageIndicatorMenuIDare not in 2e utils? >Done[2e]-comment this). >DoneID_BLOCK_UNWRAPQUOTESATCURSORis complimentingID_BLOCK_UNWRAPSELECTION, no need to add "ATCURSOR". >Done>There should be a system beep(instead of save) when clicking on a toolbar button 'Save settings on Exit' if application was executed without ini-file. The case is not available now because this toolbar button is disabled when 'save settings'-command is forbidden.
// Auto close tags) and reuse it in Edit.c'sEditInsertTagDlgProcinstead of the serious code duplication. >DoneSSM_REGULARtoSSM_ALLsince it really saves all settings. >DoneIsExpressionEvaluationEnabledandGetExpressionTextRange? >Doneint2binand two functions after it should be moved to 2e utils. >DoneUpdateStatusbarshould be made a 2e function. >Donenid.szTipwithWC_NOTEPAD2. >DoneNotepad2.rc
Wrap &Selection Unwrap Bra&ckets Unwrap &Quotes
Str&ip HTML Tags Escape HTML Ta&gs St&ring To Hex Hex To Stri&ng
Highlight Curren&t Word
Evaluate Math E&xpressions
&Ctrl+Arrow Navigation &Ctrl+Wheel Scroll
>The code was added back in 2013 (https://github.com/ProgerXP/Notepad2e/commit/a03ee65020565029df463072c689625c7d687aca). I've found no difference after removing the block in my local tests and see no reason to preserve it.