search

brackets-archive / brackets-shell

CEF3-based application shell for Brackets.
http://brackets.io
MIT License
0 stars 0 forks source link

[CLOSED] Windows Installer changes for optional PATH and context menu updating along with command line script addition. #343

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by nethip Tuesday Apr 14, 2015 at 13:56 GMT Originally opened as https://github.com/adobe/brackets-shell/pull/514

This change involves the following

1) Made the update path and context menu addition to file and folder context menus optional. 2) The PATH varible will now be [INSTALLDIR]/command. 3) Changed project setup scripts to create commands/brackets.bat by copying it from scripts/brackets.bat 4) Changed the Gruntfile to copy the new folder command to Brackets staging area.

nethip included the following code: https://github.com/adobe/brackets-shell/pull/514/commits

core-ai-bot commented 3 years ago

Comment by nethip Tuesday Apr 14, 2015 at 14:05 GMT

@MarcelGerber @le717 @peterflynn Please review this.

CC @ryanstewart

core-ai-bot commented 3 years ago

Comment by nethip Wednesday Apr 15, 2015 at 10:10 GMT

These are the screen shots of the installer window as well the new context menus that get installed.

New Installer Screen

newinstallerwindow

Context Menus

File Context Menu filecontextmenu

Folder Context Menu foldercontextmenu

core-ai-bot commented 3 years ago

Comment by nethip Wednesday Apr 15, 2015 at 10:16 GMT

Also you could try installing from the link below. https://www.dropbox.com/s/28zok1cjz0uxkkn/Brackets%20Release%201.3.zip?dl=0

core-ai-bot commented 3 years ago

Comment by le717 Wednesday Apr 15, 2015 at 15:53 GMT

Thanks for the screenshots and test installer. I'll try to look at this for you today, @nethip.

core-ai-bot commented 3 years ago

Comment by nethip Wednesday Apr 15, 2015 at 17:18 GMT

@le717 Sure thanks!

core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 05:29 GMT

@peterflynn If these changes are OK could we go ahead with the merge. @le717 Did you get a chance to play around with the build. Let me know in case of any issues.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Apr 16, 2015 at 08:26 GMT

@nethip The UI here looks great. However, when I installed this on my Win 8 machine it did not appear to modify the PATH, and typing brackets in a command prompt doesn't work. Is a reboot supposed to be needed or anything? Fwiw, I unchecked Explorer menu, but left PATH checked.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Apr 16, 2015 at 08:27 GMT

Actually, if I open an admin command prompt it works. Command prompts without admin privileges (i.e. the most common kind) have a different PATH though, without Brackets included. That seems like a blocker for landing this :-/

core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 08:42 GMT

@peterflynn I just checked. In the later case the Path is getting picked up from System variables PATH. In your first case PATH got picked from user variables PATH. Does it make sense to add PATH to both system and user variables?

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Apr 16, 2015 at 08:45 GMT

It absolutely needs to work in the normal command prompt (people almost never run a UAC-elevated command prompt), so I'd say yes. Either that or only add to the user one and skip the system one altogether?

core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 08:48 GMT

@peterflynn Actually the change is very simple. In Brackets.wxs we just need to change this System="yes" to System="no". This is how it will now look. Let me quickly check this and get back to you. Thanks for catching this!

<Environment Id="PATH"
                               Action="set"
                                Part="last"
                                Name="PATH"
                                Permanent="no"
                                System="no"
                                Value="[INSTALLDIR]command" />
core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 09:33 GMT

@peterflynn I think a reboot is still required. I changes both these settings and on some machines the PATH is not getting updated until rebooted. Can you confirm this. I checked this on some systems.

core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 14:01 GMT

@peterflynn @ryanstewart I figured out what the problem was. There is a problem with MSI built with WIX to not propagate the PATH environment variable change to the explorer. In order to solve this problem one has to reboot the system. This has been a classic problem and there are lot of threads on stack overflow to overcome this problem. Everyone suggests custom actions.

So in order to solve this problem I have bootstrapped a C++ module to MSI, which gets called after the installation is finished. This C++ routine basically broadcasts a WM_STATE_CHANGE message, so that the explorer knows it needs to refresh its environment. I have created a PR for this.

https://github.com/adobe/brackets-shell/pull/515. I have raised this to be merged with this PR rather than master.

In this PR you will find a DLL that contains the C++ routine. I built this DLL from my local system and having this part of the grunt job is a lot of effort. So for now we could ship this as DLL in our repo. I will add grunt tasks to build this DLL in next release/may be just post the project file at another repo location and add instructions to build and continue with building our installer.

The following is the C++ code. It would be great if someone can do a quick review of this.


UINT __stdcall RefreshEnvironmentVariables(MSIHANDLE hInstall)
{
    HRESULT hr = S_OK;
    UINT er = ERROR_SUCCESS;

    hr = WcaInitialize(hInstall, "RefreshEnvironmentVariables");
    ExitOnFailure(hr, "Failed to initialize");

    WcaLog(LOGMSG_STANDARD, "Initialized.");

    // Send out the Settings Changed message - Once using ANSII...
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

    // ...and once using UniCode (because Windows 8 likes it that way).
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)L"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

LExit:
    er = SUCCEEDED(hr) ? ERROR_SUCCESS : ERROR_INSTALL_FAILURE;
    return WcaFinalize(er);
}

If any one is interested, here is the link from which I took this code. http://www.kajabity.com/2013/09/an-update-to-the-apache-maven-installers-broadcasting-wm_settingchange-to-update-environment-variables/

core-ai-bot commented 3 years ago

Comment by nethip Thursday Apr 16, 2015 at 14:06 GMT

Also I have posted this reposted this build again at the following location.

https://www.dropbox.com/s/28zok1cjz0uxkkn/Brackets%20Release%201.3.zip?dl=0

@peterFlynn @le717: Could you please install the build from the new location and let me know if you are running into any issues.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Apr 16, 2015 at 20:37 GMT

@nethip Hmm, I'm confused -- in the previous build, the command prompt did reflect the variable right away (without reboot) but only when using the admin PATH. If we change to setting the non-admin path instead, why would that behave any differently?

Note there's a difference between:

  1. Existing command prompts that are already open (before running installer) update to reflect the new PATH (without needing to reboot or close/reopen the command prompt)
  2. Command prompts you open after running the installer reflect the new PATH (without needing to reboot)

It seems like we already had (2) working. It's very unlikely users expect (1) to work, since it basically never works like that when anything else updates the PATH.

This answer seems to agree:

Any programs spawned via Explorer after this should get the updated environment, although already-running programs will not, unless they handle the setting change message.

In other words, we shouldn't have to do anything special to (2) to work -- the extra WM_SETTINGCHANGE is only needed if we want (1) to work, which I think is overkill since no one else does that.

core-ai-bot commented 3 years ago

Comment by le717 Thursday Apr 16, 2015 at 22:06 GMT

I'm downloading and installing the new installer right now. TBH, I'm still not totally sold on adding Brackets the the PATH, for as we've said, literally no other Windows IDE does this.

core-ai-bot commented 3 years ago

Comment by le717 Friday Apr 17, 2015 at 00:24 GMT

I just took at look at this. I'll edit this message with my response, so check back in a short bit. ;)

core-ai-bot commented 3 years ago

Comment by le717 Friday Apr 17, 2015 at 00:28 GMT

Existing command prompts that are already open (before running installer) update to reflect the new PATH (without needing to reboot or close/reopen the command prompt). It's very unlikely users expect (1) to work, since it basically never works like that when anything else updates the PATH.

@peterflynn is correct, any command prompts currently open are not expected to get the updated environment. It's not worth extra code to do something that's not expected to happen in the first place.

However, when I installed this on my Win 8 machine it did not appear to modify the PATH, and typing brackets in a command prompt doesn't work. Is a reboot supposed to be needed or anything? Fwiw, I unchecked Explorer menu, but left PATH checked. Actually, if I open an admin command prompt it works. Command prompts without admin privileges (i.e. the most common kind) have a different PATH though, without Brackets included. That seems like a blocker for landing this :-/

Win7 Home x64, not seeing this issue. I'll try to install the build on a Win8 machine and see what happens. No, non-admin prompts can access the system PATH (open a non-admin prompt and run echo %PATH%). The only different between the two paths are system wide (all user accounts) vs. current user account only. If I had to vote for which one to use, I'd say system unless there's a reason to restrict it to current user, which in that case there should be an exclusive option for the user to choose between the two, if possible).

The option to add to the context menu is great, and defaulting that check is (arguably) a good idea (I'll go for it). I'm a little iffy on the wording for both checkboxes, but I'll comment on that separately. :) Using the batch script for the launcher is (and having it under /command) is much better. As I said, I'm still not sold on the PATH idea, and really we don't have feedback from other Windows users on this. How about leaving it unchecked by default and once we gain some feedback on it, we can decide on default checking it?

Everything else looks pretty good. I'll comment on those wording then see about trying this on Win8. :)

core-ai-bot commented 3 years ago

Comment by le717 Friday Apr 17, 2015 at 01:10 GMT

@peterflynn Just ran the installer on Win8.1 Pro x64, couldn't repo your bug. :\

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 02:55 GMT

@peterflynn @le717 I tested this with multiple systems(tested on 3 systems all running Win 7 64 bit) and I am able to repro this issue consistently. I have not tried this on Win 8 though. It does not matter whether command prompt is open or not or if a new cmd is opened after the installation. The only way some one can solve this problem is to restart the system. So the answer @peterflynn is referring to is to do with changing the env variables manually, rather programatically. Programs, like msiexec, need to broadcast WM_SETTINGSCHANGE in order for the explorer the refresh the environment, which in our WIX case is not getting done for some strange reason.

@peterflynn I ran into exactly your case where immediately after I install I was able to use brackets from command prompt on my system. But I then figured out that it could be something with the envcaching. I was able to repro this after I uninstall Brackets, restart and install brackets again.

Here are the right steps to repro this.

After performing these steps. I will double check with a Win 8 system in the office and update you guys on that.

If we don't want to add this custom action, then we need to atleast convey the user with something like

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 03:02 GMT

@le717 About the editors which add themselves to PATH, there are a couple of them, which I see on my system itself. I can see that there are paths to Visual Studio, Perforce appended to PATH variable.

We now have minimized side effects by just adding the PATH to [INSTALLDIR]\command folder rather than to Brackets [INSTALLDIR] itself.

I still think the strings in the current form are concise and are easily understandable. Anything more verbose would be an overkill. In fact we had something similar to what you had proposed earlier before changing them to the current form.

core-ai-bot commented 3 years ago

Comment by ryanstewart Friday Apr 17, 2015 at 06:37 GMT

@peterflynn @nethip If we need to land this to get the command prompts working, let's do it. It sounds like changing that single flag wasn't enough.

@le717 I propose we go ahead and do the prerelease build with the checkbox checked by default and collect feedback over the week. I'll also send out a message to the Brackets-Dev list to get feedback. If we decide to roll it back and un-check it, that sounds like an easy fix.

@nethip If we un-check it, the only way a user would be able to get the command line tool to work would be to (a) add it to their path manually or (b) reinstall Brackets, right?

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 06:43 GMT

@ryanstewart Yup! The only way this works is by reinstalling. We could provide ways to do this in our command line wiki that if a user wants to do this manually.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Apr 17, 2015 at 07:39 GMT

@ryanstewart The main risk of adding custom native code into the installer is that any bug in this code is effectively 100% bricking Brackets for users who hit it, because they've completely unable to even finish getting it installed. Wix/MSI may be ugly and annoying, but it has been very reliable so far.

Is @nethip the only person who can repro the issue where a restart is required? It sounds like both me & @le717 are able to see the PATH update without a restart (at least in some command prompts -- see discussion above about admin vs. non-admin). Maybe we should cast a wider net and see how widely reproducible the bug is first? Or try to get a sense of what triggers people to hit it...

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Apr 17, 2015 at 07:44 GMT

@le717 I'm confused about this:

No, non-admin prompts can access the system PATH (open a non-admin prompt and run echo %PATH%). The only different between the two paths are system wide (all user accounts) vs. current user account only. If I had to vote for which one to use, I'd say system unless there's a reason to restrict it to current user

Both command prompts have a PATH variable, yes -- but they are two different values. On my Win 8 machine, echo %PATH% does not print the same value. The admin command prompt seems to reflect the system PATH while the regular command prompt seems to reflect the user path. Between the two, the user command prompt is the 99% use case, so we absolutely have to get that one right.

But you said you can't repro the bug -- do you mean both regular and admin command prompts show Brackets in the path after installing? How are you opening them? (I tried both Win-R to run cmd, and Win-X > Command Prompt). Are you sure you were using the installer before @nethip changed the code to set user path instead of system?

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Apr 17, 2015 at 07:45 GMT

Fwiw btw, I think it's fine to leave the checkboxes checked by default.

That seems to be what other apps do, and most Windows power users are pretty well trained to inspect every checkbox in an installer to make sure it's something they really want (due to the proliferation of optional adware/bloatware in installers).

core-ai-bot commented 3 years ago

Comment by ryanstewart Friday Apr 17, 2015 at 08:12 GMT

@peterflynn @nethip I was able to reproduce the behavior that Prashanth saw on my Windows 8 machine. After installing without his patch I wasn't able to use the brackets command.

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 08:49 GMT

@ryanstewart Thanks for testing this Ryan!

@peterflynn @le717 I have not changed the System="yes" to System="no", because it does not seem to effect anything.

The code that is going to be executed is pretty small. In fact the method above,RefreshEnvironmentVariabels(), was generated using Windows Installer XML wizard in VS 2010. The only two lines of code I have added are these.

    // Send out the Settings Changed message - Once using ANSII...
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

    // ...and once using UniCode (because Windows 8 likes it that way).
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)L"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

These are pretty safe and also the SMTO_ABORTIFHUNG flag takes care of any waiting issues. Custom Actions are something provided by Wix framework itself. And considering this is provided by Wix framework, it should be safe to use them. There are lot of Wix developers, who rely a lot on custom actions.

Now that @ryanstewart had hit this too, looks like this is more of a common scenario and needs to be fixed.

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 10:28 GMT

@peterflynn Based on your comments, I have now reduced the failure conditions by always flagging a success in the custom code. Here is the new code.

UINT __stdcall BroadcastEnvironmentVariablesChange(MSIHANDLE hInstall)
{
    HRESULT hr = S_OK;
    UINT er = ERROR_SUCCESS;

    hr = WcaInitialize(hInstall, "BroadcastEnvironmentVariablesChange");
    ExitOnFailure(hr, "Failed to initialize");

    WcaLog(LOGMSG_STANDARD, "Initialized.");

    // Send out the Settings Changed message - Once using ANSII...
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

    // ...and once using UniCode (because Windows 8 likes it that way).
    SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)L"Environment", SMTO_ABORTIFHUNG, 5000, NULL);

LExit:

    er = SUCCEEDED(hr) ? ERROR_SUCCESS : ERROR_INSTALL_FAILURE;
    if (er == ERROR_INSTALL_FAILURE)
        WcaLog(LOGMSG_STANDARD, "Custom action to broadcast environment variables change has failed!");

    // Since this custom action is not application critical custom action, we will ignore any failures.
    return WcaFinalize(ERROR_SUCCESS);

}

I would like to see if we are running into problems by getting this tested by larger audience. As we are doing a prerelease build today, I think we should use this as an opportunity to see if we are running into more problems, with this new change.

I could then work with @peterflynn and @le717 for further changes, that would make it to the final release.

core-ai-bot commented 3 years ago

Comment by nethip Friday Apr 17, 2015 at 11:10 GMT

I am closing this PR so that we can re-target it to release.