Add JNA code to retrieve default browser from Windows registry#6626
Closed
dragonsKnight5 wants to merge 1 commit intoapache:masterfrom
Closed
Add JNA code to retrieve default browser from Windows registry#6626dragonsKnight5 wants to merge 1 commit intoapache:masterfrom
dragonsKnight5 wants to merge 1 commit intoapache:masterfrom
Conversation
mbien
added
the
ci:dev-build
mbien
reviewed
Oct 26, 2023
Member
mbien
left a comment
mbien
left a comment
There was a problem hiding this comment.
congrats for figuring out the encoding/line ending issue, please see comments inline
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); |
Member
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; | ||
| } |
Member
There was a problem hiding this comment.
remove all .toUpperCase() from here since it is already in upper case at this point.
Author
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; | ||
| } |
Member
There was a problem hiding this comment.
its ok to return null here since empty string isn't useful either
Author
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(); | ||
| } | ||
|
|
Member
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.
Author
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
Author
|
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; |
Contributor
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); |
Contributor
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
Author
|
I have now corrected these syntax errors, I feel like a bit of an idiot for missing these 😬 |
ebarboni
reviewed
Nov 6, 2023
|
|
||
| import com.sun.jna.platform.win32.Advapi32Util; | ||
| import com.sun.jna.platform.win32.WinReg; | ||
| import java.util.Locale |
Contributor
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; |
Contributor
There was a problem hiding this comment.
still missplaced . should be userChoice.contains
Member
|
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
Author
|
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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"