Support data interpreter show / hide

[datasynergyuk]

Also, minor correction to capitalisation of "Ascii" abbreviation to "ASCII"

[jovibor]

IDM_HEXCTRL_DATAINTERPRET_HIDE was added without any menu assigned. That's something that's not right in principle.

Following the IsDataInterpretVisible logic we should add the similar for Search, FillData, Bookmark manager, and the rest dialogs. I don't think it's a generally good/helpful idea. Must be something else.

Also, you added two absolutely unrelated commits in one PR (ascii and dialog logic). I extarcted ae83aea and added it separately.

Please, make PRs more atomic. One PR for one associated/related changes.

[datasynergyuk]

Thank you for your feedback. I think maybe this is an example of the old proverb Perfect is the enemy of good.

IDM_HEXCTRL_DATAINTERPRET_HIDE was added without any menu assigned. That's something that's not right in principle.

I suggested this feature because having multiple data interpreter dialogs is a real problem when the control is integrated into a (multi-document) parent application. It therefore becomes important that the user can associate the data interpreter with the correct HexCtrl. One way to do this is to hide/unhide the interpreter so that only one instance of it is visible and easily associated with the current document.

I would be happy to add a checkbox against the context menu item and re-arrange it so that it hides/shows the interpreter. Would this be acceptable?

NB: I didn't make this change to the control in the original PR because it isn't strictly necessary when the control is used standalone.

Following the IsDataInterpretVisible logic we should add the similar for Search, FillData, Bookmark manager, and the rest dialogs. I don't think it's a generally good/helpful idea. Must be something else.

Yes. Similar logic could be used for the other dialogs and maybe, if someone found it useful, they would want to spend time to add these features. I may do this myself if I have time. However, in the meantime, please don't let perfect be the enemy of the good. I considered adding a parameter to the message to define if the dialog should be made visible / hidden. However, the current ExecuteCmd() framework doesn't use the lparam for anything. Further, after checking the code, I found that the messages IDM_HEXCTRL_MODIFY_UNDO and IDM_HEXCTRL_MODIFY_REDO already follow a similar pattern. This is why I chose to implement it as a new message rather than by refining an existing method. Maybe it would be better to support an additional parameter in ExecuteCmd() and reduce the number of messages accordingly.

Also, you added two absolutely unrelated commits in one PR (ascii and dialog logic). I extarcted ae83aea and added it separately. Please, make PRs more atomic. One PR for one associated/related changes.

Understood although you did the same thing in the subsequent commit

[jovibor]

Thank you for your feedback. I think maybe this is an example of the old proverb Perfect is the enemy of good.

Where you do see the pursuit of perfection i see a simple common sense.

However, the current ExecuteCmd() framework doesn't use the lparam for anything. Further, after checking the code, I found that the messages IDM_HEXCTRL_MODIFY_UNDO and IDM_HEXCTRL_MODIFY_REDO already follow a similar pattern. This is why I chose to implement it as a new message rather than by refining an existing method. Maybe it would be better to support an additional parameter in ExecuteCmd() and reduce the number of messages accordingly.

See, this suggestion is more like long-term improvement rather than some short-term crutch. Why don't to begin with it from the very inception?

Understood although you did the same thing in the subsequent commit

I do it also for many other commits. But i'm talking here about PR, not commits.

P.S. My main message is that you try to solve issues head-on, with the first available simple solution. This is only acceptable when there is no other option available. Codebase must be supported and maintained in a long-term perspective, and if allow all kind of freedom everything will become a mess sooner or later.

[datasynergyuk]

So, given that the ability to hide the data interpreter is necessary in my use case, and you dont like my suggested approach, how would you suggest implementing this feature please?

On Tue, 21 Apr 2020, 09:53 Jovibor, notifications@github.com wrote:

Thank you for your feedback. I think maybe this is an example of the old proverb Perfect is the enemy of good https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good.

Where you do see the pursuit of perfection i see a simple common sense https://en.wikipedia.org/wiki/Common_sense.

However, the current ExecuteCmd() framework doesn't use the lparam for anything. Further, after checking the code, I found that the messages IDM_HEXCTRL_MODIFY_UNDO and IDM_HEXCTRL_MODIFY_REDO already follow a similar pattern. This is why I chose to implement it as a new message rather than by refining an existing method. Maybe it would be better to support an additional parameter in ExecuteCmd() and reduce the number of messages accordingly.

See, this suggestion is more like long-term improvement rather than some short-term crutch. Why don't to begin with it from the very inception?

Understood although you did the same thing in the subsequent commit https://github.com/jovibor/HexCtrl/commit/8a457a522123d696095622c4773bac7336dbd47b

I do it also for many other commits. But i'm talking here about PR, not commits.

P.S. My main message is that you try to solve issues head-on, with the first available simple solution. This is only acceptable when there is no other option available. Codebase must be supported and maintained in a long-term perspective, and if allow all kind of freedom everything will become a mess sooner or later.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jovibor/HexCtrl/pull/13#issuecomment-617047199, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALPLJJACAJSGOAIB575KTKLRNVNIXANCNFSM4MMPBD5A .

[datasynergyuk]

I am sorry but having looked at the code again I honestly don't see any problem with using distinct messages for show/hide. This is consistent with the existing approach for redo/undo, search next/previous, bookmark add/remove etc. In short, it would of course be possible to use a parameter to define how the "show" should be done (show or hide) but this would actually be inconsistent with all of the existing code which already uses distinct messages. There is good reason for this because, in many cases, they are distinct menu options too.

I am actually at a loss to thing of an alternative way to implement this feature if you don't like distinct messages and it is undesirable to turn the message mechanism inside out for the data interpreter special case.

[jovibor]

I am actually at a loss to thing of an alternative way to implement this feature if you don't like distinct messages and it is undesirable to turn the message mechanism inside out for the data interpreter special case.

Adding new messages into EHexCmd is absolutely fine, i did say nothing about it being wrong.

My only concern was IDM_HEXCTRL_DATAINTERPRET_HIDE without binding to any actual menu, which is by design is incorrect and bit weird.

[datasynergyuk]

Ok. Thank you.

The root of this problem is that the ExecuteCmd() interface and the underlying windows message (IDM_HEXCTRL_DATAINTERPRET) don't perfectly map together.

1. It is convenient to have distinct EHexCmd members for show/hide. However, if there is only one underlying message the show state must be passed by the lparam field. However...

2. The underlying windows message is also used by the menu. The menu cannot set the lparam field so must be interpreted as "flip" the current state.

It is difficult to align these two competing objectives. For instance, I have tried the following:

case EHexCmd::CMD_DATAINTERPRET_SHOW: wParam = IDM_HEXCTRL_DATAINTERPRET; lParam = true; break; case EHexCmd::CMD_DATAINTERPRET_HIDE: wParam = IDM_HEXCTRL_DATAINTERPRET; lParam = false; break;

However, in CHexCtrl::OnCommand(), it is difficult to decode the IDM_HEXCTRL_DATAINTERPRET message for both cases. If the lparam field is inspected this breaks the menu but if it is ignored it breaks the external interface. I cannot see how these can be aligned without having another windows message?

[datasynergyuk]

Possible solution:

1. Decouple the EHexCmd message from the windows message
2. Treat the windows message as an instruction to flip the current state
3. In ExecuteCmd() directly call the following:

void CHexCtrl::ShowDataInterpreter(bool fShow) { assert(IsCreated()); if (!IsCreated()) return;

m_menuMain.CheckMenuItem(IDM_HEXCTRL_DATAINTERPRET, fShow ? MF_CHECKED : MF_UNCHECKED);
m_pDlgDataInterpret->ShowWindow(fShow ? SW_SHOW : SW_HIDE);

}

[datasynergyuk]

Incidentally: The declaration of void CHexCtrl::ExecuteCmd(EHexCmd enCmd) as const is arguably wrong since it manifestly can make modifications to the underlying data (via the Windows message mechanism? e.g. zero data, fill data etc

[jovibor]

The root of this problem is that the ExecuteCmd() interface and the underlying windows message (IDM_HEXCTRL_DATAINTERPRET) don't perfectly map together.

They map together perfectly.

Ask yourself, what is EHexCmd? It's just a simple map (reflection) of available menus that user can click on. Nothing more. That's why ExecuteCmd(...) is so simple - just a user input emulator, kind of. Now, you're trying to put some "difficult" logic to it, that is wrong by design. That's why i've said that head-on solutions are almost always unacceptable.

2. Treat the windows message as an instruction to flip the current state

This one sounds viable, but if user clicks accidentally menu with already opened dialog it will be closed, while atm just nothing happens.

So, all in all, there is no quick/good solution for now without redesign. I will have to think about it.

[datasynergyuk]

Please see recent check in. I think this solves the problem.

On Tue, 21 Apr 2020, 12:11 Jovibor, notifications@github.com wrote:

The root of this problem is that the ExecuteCmd() interface and the underlying windows message (IDM_HEXCTRL_DATAINTERPRET) don't perfectly map together.

They map together perfectly.

Ask yourself, what is EHexCmd? It's just a simple map (reflection) of available menus that user can click on. Nothing more. That's why ExecuteCmd(...) is so simple - just a user input emulator, kind of. Now, you're trying to put some "difficult" logic to it, that is wrong by design. That's why i've said that head-on solutions are almost always unacceptable.

2. Treat the windows message as an instruction to flip the current state

This one sounds viable, but if user clicks accidentally menu with already opened dialog it will be closed, while atm just nothing happens.

So, all in all, there is no quick/good solution for now without redesign. I will have to think about it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jovibor/HexCtrl/pull/13#issuecomment-617112613, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALPLJJBXAVWTK7TSQDOFOGDRNV5OFANCNFSM4MMPBD5A .