Launcher improvements

[DRSDavidSoft]

General Improvements to the Launcher code

[WIP] - do not merge yet.

1. ✓ Move strings to separate strings.rc2 resource file

Strings that aren't short will be migrated to here instead of being defined in the C++ code like this:

cmder/launcher/src/CmderLauncher.cpp

Lines 300 to 303 in 49da374

	{
	MessageBox(NULL, L"Unrecognized option for /REGISTER or /UNREGISTER. Must be either ALL or USER.", MB_TITLE, MB_OK);
	exit(1);
	}

2. ✓ Switch to TaskDialog instead of MessageBox for displaying messages

Since XP has been dropped (#1707), we can switch to TaskDialog to deliver a better message window, as discussed in #1726 (comment)

3. Move functions to a class – TODO

TBA – General refactoring of the code.

4. Detect and configure %CMDER_ROOT% variable – TODO

TBA – When registering the application and the context menu, Cmder will check for and correct %CMDER_ROOT% if needed.

5. Add switches for /version and /help – TODO

TBA – Useful to quickly check for the current binary version.

[DRSDavidSoft]

@daxgames @Stanzilla I need your opinion on some topics:

1. I'd like to implement a "check for updates" feature.

   • Cmder should only check for updates
   • Cmder should be able to automatically update to a new version
   • This feature should not be implemented.

2. Should I add a confirmation dialog for:

   • Ask the user before running /register and /unregister switches
   • Warn and ask the user to fix %CMDER_ROOT% if it's set to an invalid value
   • Ask the user to check for updates

3. If there is a "do not as me again" checkbox,

   • It should be saved in a config file (How to store configuration #1695)
   • It should be saved in the registry
   • Other?

[daxgames]

I am not a c++ dev so I can't tell you how the code should or should not be organized. At best I hack at it until I get the result I want, so have at it.

I'm not sure how %cmder_root% could be wrong since it is based on the exe dir of the launcher exe. Not sure what you mean by discover and configure.

I don't think there needs to be a check for update feature nor an auto update feature. Its a portable app you download, expand and copy it to the dir, it's just not necessary. This is probably the best idea of them all, while I think it's unnecessary and could be confusing since conemu already has this and it is often confused with a cmder updater users may like it.

Why would you add a confirmation dialog for a command line switch the user typed, isn't that confirmation enough.

No checkboxes, no registry settings, no more config files. Btw what does a 'do not as me again box' do? Is it for the proposed features like check for update? If you are storing additional config it should be in a file in %cmder_root%/config

[daxgames]

Actually I'd like the version file to be called version and contain the version instead of version-1.3.6.675 so that when someone upgrades they don't end up with multiple version files. A /h and a /v switch might be nice and easy to add.

[DRSDavidSoft]

Thanks for your input, @daxgames.
Here's my explanations:

Variable detection

I'm not sure how %cmder_root% could be wrong since it is based on the exe dir of the launcher exe. Not sure what you mean by discover and configure.

There could be possible reasons why the content of the variable may not have a correct value:

• The directory has been moved (e.g. from Downloads to Desktop)
• The mounting partition could change (e.g. the memory stick could change from E: to G:)
• A previously defined variable could interfere (e.g. There's a system variable defined, but we need to set a user variable)

The plan is to check the content of the variable upon startup, and display the following message when an incorrect value is detected.

Variable warning	Variable warning (more details)

BTW, one use of 'Do not ask me again' is here (the checkbox in the bottom.)

Check for updates

I don't think there needs to be a check for update feature nor an auto update feature. Its a portable app you download, expand and copy it to the dir, it's just not necessary. This is probably the best idea of them all, while I think it's unnecessary and could be confusing since conemu already has this and it is often confused with a cmder updater users may like it.

I understand, however I do still suggest a behaviour that matches the portable downloaded apps aspect. Just running a simple check for updates (and linking users to the update page where they can download themselves.) Simply put, if there is an update on GitHub releases, a simple message could be shown, asking the use if they'd like to open the download page.

As for the ConEmu feature, I think its Update.CheckOnStartup should be set to disabled in Cmder if it's not already. This will help prevent confusion.
The plan is to write a good update routine in the Cmder Launcher itself that takes care of compatibility check, configuration backups, and confirmations / notifications for the user.

Command-line confirmation

Why would you add a confirmation dialog for a command line switch the user typed, isn't that confirmation enough.

Hmm, you right about the user typed switches already being confirmations. The idea was to use the /register and /unregister in the unofficial setup scripts (i.e. 'Uninstall' would call `Cmder.exe /unregister) which would ask the user if they wanted to remove Cmder.

I think my thinking was not valid. Those confirmation pages should go in those scripts directly, and calling any switches should perform the action silently.

How to store settings

Btw what does a 'do not as me again box' do? Is it for the proposed features like check for update?

Any dialog where the user may choose an action 'as always', including check for updates.

Now, a follow up question: If I implement this, should I define separate flags for each routine (e.g. downloads, checks, etc), or should they all follow a single 'do not ask me again' directive?

No checkboxes, no registry settings, no more config files. [...] If you are storing additional config it should be in a file in %cmder_root%/config

Ouch! I mean I still agree that the location of the config should be in %cmder_root%/config, but I'm not sure on how to store the individual settings for the launcher. Do I use launcher.txt? launcher.ini? launcher.xml? (See #1695 (comment))

Just a thought though, if we are already modifying the Registry settings for the context menu, IMHO it means that it's very likely that the user is using Cmder in non-portable mode.

The 'version-*' file(s)

Actually I'd like the version file to be called version and contain the version instead of version-1.3.6.675 so that when someone upgrades they don't end up with multiple version files

👍 on the idea. I also dislike having multiple version-* files in the root directory after an update.

However, actually the file's name is being read by the user to check for the version of Cmder, not its content in a text reader. I'd vote to use #1729's method instead of opening the file with an editor, which is easier and more consistent.

I'd suggest either:

1. Getting rid of the file completely and use Cmd.exe's properties to check for version instead
2. Writing a simple routine to remove older version-* files in the root directory

[daxgames]

@DRSDavidSoft Sorry if I was harsh. It was a long day. I understand your use cases and actually like the dialog mock ups. The examples talk about the %CMDER_ROOT% variable which I still don't see how it could be wrong but I can see how the variable based on cmder.exe current path could be at odds with a previously registered context menu settings set from another path. It could become VERY annoying though to someone like me that has multiple copies of cmder installed for testing purposes.

Maybe I'm old school but xml is probably overkill to store a set of binary flags to save checkbox state. Ini file would probably suffice and I would do a separate flag for each checkbox. I prefer file storage to registry storage because at its core cmder is a portable app. I don't even enable the context menu until someone complains its not working and I have to test/fix something. When I'm done I disable it.

My biggest concern with your ideas is maintainability. Seems like a lot to add to an otherwise simple launcher. We barely support the launcher today because none of us are really c++ experts. You may know it but what if you get bored with the exciting world of contributing to cmder and decide your done.

[DRSDavidSoft]

@daxgames It's okay, I actually like your counter arguments.

The examples talk about the %CMDER_ROOT% variable which I still don't see how it could be wrong

IMO, The one use case (moving it around when the context menu is registered) justifies the reason.
However, if you don't see how it could be wrong, neither the user will. If the variable is really correct, the code will simply ignore and pass the warning; but if the variable is either changed or set incorrectly, a simple warning will pop up asking the user if they want Cmder to fix it. I see no UX harm in that.

It could become VERY annoying though to someone like me that has multiple copies of cmder installed for testing purposes.

Solution: Do not ask me again :) (either ticking the checkbox, or a simple .ini line, whichever is the simplest).

I have multiple copies of Cmder as well (I think for each branch that I'm working on, I have one)

So once (or if) this PR gets merged, the annoyance will be only be setting that 'Do not ask me again' flag once.

Maybe I'm old school but xml is probably overkill to store a set of binary flags to save checkbox state. Ini file would probably suffice and I would do a separate flag for each checkbox.

I agree with you, XML is actually overkill just to store some flags. I also prefer a .ini file to store them. (The .ini files also go really well with Win32 c++ programs!)
The main concern was scalability, if the launcher was ever to incorporate more features some day, but I'm already thinking against that.

I prefer file storage to registry storage because at its core cmder is a portable app.

Couldn't agree more. Portability is the #1 feature of Cmder.

---

Now about the maintainability discussion

My biggest concern with your ideas is maintainability. Seems like a lot to add to an otherwise simple launcher. We barely support the launcher today because none of us are really c++ experts. You may know it but what if you get bored with the exciting world of contributing to cmder and decide your done.

I have said before I am concerned with adding so much functionality to the launcher that it becomes difficult to maintain.

I completely understand your point, I wouldn't want having a lot of unmanageable code added to my repo either. However, I should say that this launcher wouldn't have been possible if @austinwagner didn't made a PR, or if he did and @samvasko didn't approve it. Many contributors to any project come and go, and they build upon the project as they come.

I don't consider myself a C++ expert. As you said, I just know how to do it. But it's against my nature to toy with something and get bored with it. I might stop contributing one day, but I won't leave a ton of unfinished, unmaintained code in the repo when I do it. Furthermore, that day isn't today. I can assure you I'll have interest in maintaining the features that I introduce for many more days to come.

The only issue that remains IMHO, is to decide what features to add, and which ones to drop. I won't introduce any unnecessary changes into the launcher code.

So, as long as we sort out the inevitable issues that arise with each new feature, I would ask that to be added to Cmder.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contribution.

[DRSDavidSoft]

↩️

[MartiUK]

@DRSDavidSoft is this still WIP and unable to merge?

[DRSDavidSoft]

@MartiUK I'm sorry, but I have committed a lot of changes that need to be pushed before this PR is ready to be merged.

I haven't had time to polish and test my changes (in addition to resolving a couple of conflicts), so please keep this on hold until I can complete everything.

I'll post an update when the changes are ready to be pushed. Thanks for the patience!

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contribution(s).

[DRSDavidSoft]

I'm temporarily closing this PR as I've been busy in the past couple of months, and don't want the bot to close it as stale.

Once I finish working on it, I will reopen the PR.

[MartiUK]

@DRSDavidSoft Okay mate, no rush

_{Sent with GitHawk}