Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WinGUP Enhancement npp #2

Conversation

Projects
None yet
2 participants
@SinghRajenM
Copy link

commented Aug 11, 2018

This PR improves WinGUP in below fashion –

  1. Provide an option to pass commandline parameter for installer (includes both normal or silent mode paramters)
  2. Provide an option to update app in background.
    a. If app is opened, then it will for app to exit.
    b. App installer must support silent installation and silent mode parameter should be added to configuration file. (e.g. /S flag which is default if flag is missing in configuration).
    c. Default silent mode (flag /S) will be used if it is missing in configuration file.
  3. Provide a way to set icon (taskbar and app icon) via configuration file. As of now, it uses window default icon which makes it hard identify what kind of process it is.
  4. Provide a way to find Proxy dialog if it goes behind any app. As of now proxy dialog does not create any icon on taskbar. Therefore, it is hard to find the dialog once it is hidden behind some app.
  5. Provide taskbar icon to progress dialog and it’s messagebox dialog. Like point 2 and point 3.
  6. Fixes small glitch of curl. In very beginning curl returns garbage value to its CURLOPT_PROGRESSFUNCTION method which turns as showing invalid download percentage (e.g. 9223372036854775808%).
  7. Once downloading is completed, close the progress dialog as there is no need to keep it opened because either new messagebox to close the app or installer will appear based on classname is specified or not in the configuration.

image
image

Comparison report

@SinghRajenM SinghRajenM force-pushed the SinghRajenM:src_EnhanceWinGUPForNpp branch from a9ffc5c to 270a524 Aug 11, 2018

@donho

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@SinghRajenM (Some) Nice features! However, there may be some features which won't be accepted. In anyway this PR won't be accepted because there are at least 3 features in the same PR.

In order to make code review easier, please do one feature per PR to make your PR easier to be accepted.

Thank you for your contributions!

@donho donho closed this Oct 9, 2018

@SinghRajenM

This comment has been minimized.

Copy link
Author

commented Oct 9, 2018

@donho This PR is Basically has two type of features

*Major Feature:

  1. Provide options for background update
  2. Provide icon for WinGUP.

*Minor Feature:

  1. Provide commandline support for installer.
  2. Workaround for curl bug
  3. Close progress bar once downloading is over.

We have to provide commandline support if we are going to execute installer in background, because some app may use commandline other than "/S".

So if we come to code review,
Minor feature 1, can't be excluded. So we can consider it as part of Major feature 1. Moreover, not much changes also. Only File gup.xml and xmltool.cpp (ofcource xmltool.h)
Minor feature 2 and 3 are handled static size_t setProgress(HWND, double t, double d, double, double) which is fairly easy to review.

So considering above points, we can say this PR actually deals with two major features.

Now, if we come to code review for these features.
Major Feature 2: Setting icons (for all dialogs or messageboxes except progress dialog), will be handled by statement CUXHelper uxHelper; as it class CUXHelper constructor hooks callback for current thread. while for "progress dialog", static method called, because "progress dialog" is created in seperate thread.

Rest of code is for supporting background mode installation.

However, there may be some features which won't be accepted.

Let me know which will not be accepted, I will remove them and let us make most waited feature (background installation: I feel) happen.

@donho

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@SinghRajenM For me "Provide icon for WinGUP." looks cool but it's nice to have so it depends on its complexity. The others, why not? and once again, it depends on the complexity of modification.
As a result, it'll be better to split PR by per feature so I can do the better decision.
Thank you for your explanation.

@SinghRajenM SinghRajenM deleted the SinghRajenM:src_EnhanceWinGUPForNpp branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.