Add JNA code to retrieve default browser from Windows registry #6626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbien
added
the
ci:dev-build
mbien
reviewed
Oct 26, 2023
Comment on lines
+141
to
+148
| String userChoice = Advapi32Util | ||
| .registryGetStringValue( | ||
| WinReg.HKEY_CURRENT_USER, | ||
| "SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\https\\UserChoice", | ||
| "ProgId" | ||
| ) | ||
| .toUpperCase(Locale.ROOT); |
There was a problem hiding this comment.
this will throw a NPE if registryGetStringValue(...) returns null
change to:
String userChoice = Advapi32Util
.registryGetStringValue(
WinReg.HKEY_CURRENT_USER,
"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\https\\UserChoice",
"ProgId"
);
if (userChoice == null || userChoice.trim().isEmpty()) {
return ExtWebBrowser.IEXPLORE;
} else {
userChoice = userChoice.toUpperCase(Locale.ROOT);
}
Comment on lines
150
to
166
| if (userChoice.toUpperCase().contains(ExtWebBrowser.FIREFOX)) { | ||
| return ExtWebBrowser.FIREFOX; | ||
| } | ||
| else if (userChoice.toUpperCase().contains(ExtWebBrowser.CHROME)) { | ||
| return ExtWebBrowser.CHROME; | ||
| } else if (userChoice.toUpperCase().contains(ExtWebBrowser.CHROMIUM)) { | ||
| return ExtWebBrowser.CHROMIUM; | ||
| } else if (userChoice.toUpperCase().contains(ExtWebBrowser.MOZILLA)) { | ||
| return ExtWebBrowser.MOZILLA; | ||
| } else { | ||
| return ExtWebBrowser.IEXPLORE; | ||
| } |
There was a problem hiding this comment.
remove all .toUpperCase() from here since it is already in upper case at this point.
There was a problem hiding this comment.
When I backed up my fork before deleting and reforking netbeans I've somehow managed to grab the previous code configuration for this function 😫
Now fixed again
Comment on lines
177
to
182
| // ensures a null value is never returned | ||
| if (executionCommand == null) { | ||
| return new String(); | ||
| } else { | ||
| return executionCommand; | ||
| } |
There was a problem hiding this comment.
its ok to return null here since empty string isn't useful either
There was a problem hiding this comment.
I was hoping to avoid causing a null related error, I have changed it back to how it was before
Comment on lines
215
to
283
| String cmd = getDefaultOpenCommand (); | ||
| String cmd = getDefaultOpenCommand(); | ||
|
|
||
| /** if not found with getDefaultWindowsOpenCommand function | ||
| * fallback to previous method | ||
| */ | ||
| if (cmd.isEmpty()) { | ||
| cmd = getDefaultOpenCommand(); | ||
| } | ||
|
|
There was a problem hiding this comment.
the same method is called twice, something is wrong here.
this is probably supposed to be:
String cmd = getDefaultWindowsOpenCommand();
if (cmd == null || cmd.trim().isEmpty()) {
cmd = getDefaultOpenCommand();
}if the fallback is still required needs to be discussed. My feeling is that it does the same thing just via native code, if true, it should be removed in my opinion since it would be just a redundant call giving the same results.
There was a problem hiding this comment.
I think I was in to much of a rush to resubmit the pull request that I managed to missed this
The previous method tries to work out the default browser by looking in HKEY_CLASSES_ROOT at .html, the new method finds it by looking in HKEY_CURRENT_USER at the userChoice for https.
dragonsKnight5
force-pushed
the
master
branch
from
639f41a to
33a9cb8
Compare
|
I believe I have corrected all of my id10t errors. Microsoft have depreciated internet explorer, do I need to add any thing to deal with this? Here's the depreciation announcement link |
ebarboni
reviewed
Nov 3, 2023
Comment on lines
160
to
162
| } else if (userChoice).contains(ExtWebBrowser.MOZILLA)) { | ||
| return ExtWebBrowser.MOZILLA; |
There was a problem hiding this comment.
userChoice). should be userChoice.
ebarboni
reviewed
Nov 3, 2023
| "SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\https\\UserChoice", | ||
| "ProgId" | ||
| ) | ||
| .toUpperCase(Locale.ROOT); |
There was a problem hiding this comment.
need import java.util.Locale
ebarboni
reviewed
Nov 3, 2023
| else if (userChoice.contains(ExtWebBrowser.FIREFOX)) { | ||
| return ExtWebBrowser.FIREFOX; | ||
| } | ||
| else if (userChoicecontains(ExtWebBrowser.CHROME)) { |
dragonsKnight5
force-pushed
the
master
branch
from
33a9cb8 to
bfc2731
Compare
|
I have now corrected these syntax errors, I feel like a bit of an idiot for missing these 😬 |
ebarboni
reviewed
Nov 6, 2023
| @@ -19,6 +19,10 @@ | |||
|
|
|||
| package org.netbeans.modules.extbrowser; | |||
|
|
|||
| import com.sun.jna.platform.win32.Advapi32Util; | |||
| import com.sun.jna.platform.win32.WinReg; | |||
| import java.util.Locale | |||
There was a problem hiding this comment.
missing ; at end of line. You should try to compile using ant to get rid of the "basic compiler issue"
ebarboni
reviewed
Nov 6, 2023
Comment on lines
157
to
158
| else if (user.Choicecontains(ExtWebBrowser.CHROME)) { | ||
| return ExtWebBrowser.CHROME; |
There was a problem hiding this comment.
still missplaced . should be userChoice.contains
|
please make sure that it builds and works as expected locally first, before requesting reviews. Reviews are not there to fix compiler errors which can be easily found locally. |
update testGetDefaultOpenCommand to use new JNA based method update testGetDefaultOpenCommand to use new JNA based method update testGetDefaultOpenCommand to use new JNA based method Add JNA code to retrieve default browser from Windows registry add JNA dependency Add JNA code to retrieve default browser from Windows registry correct ID10t errors made by me Add JNA code to retrieve default browser from Windows registry fix syntax errors fix errors Add JNA code to retrieve default browser from Windows registry
dragonsKnight5
force-pushed
the
master
branch
from
bfc2731 to
8bcb2b0
Compare
|
I honestly thought I had rectified all errors, it's not my intention to make a nuisance of myself I've put the build log below as proof that it compiles and runs |
mbien
added
Platform
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
changes the default browser selection behaviour when executed on Windows to use the users set default browser
related to issue "Default browser on Windows #3960"