Show a dedicated message when mounting to an occupied Windows drive #2961
Conversation
|
|
| @@ -97,7 +97,9 @@ private Runnable prepareMountPoint() throws IOException { | |||
| } | |||
| } else { | |||
| var mpIsDriveLetter = userChosenMountPoint.toString().matches("[A-Z]:\\\\"); | |||
| if (!mpIsDriveLetter && canMountToParent && !canMountToDir) { | |||
| if (mpIsDriveLetter) { | |||
| if (driveLetters.getOccupied().contains(userChosenMountPoint)) throw new IllegalMountPointException("Drive \"" + userChosenMountPoint + "\" is already occupied."); | |||
There was a problem hiding this comment.
This string probably needs to use a resource for translation. I'd appreciate any pointers how to do that properly, as Mounter does not seem to have access to the resource bundles.
There was a problem hiding this comment.
Sure:
- Add a parameter
ResourceBundle resourceBundleto the constructor, so you can assign it to a private final field - Re-run the Maven build, so that Dagger will generate the glue code used to inject the resourceBundle
- Add english (and only english) strings to
src/main/resources/i18n/strings.properties- all other languages will be translated later and must not be touched
There was a problem hiding this comment.
That said, this is the wrong place for translations. We don't translate exception messages but only the UI.
What I would prefer instead: Either create some specific IllegalMountPointException subclasses (we could even make this a sealed class hierarchy 🤔) or add some machine-readable cause (use an enum) to the exception, which can then be passed to the UI in org.cryptomator.ui.unlock.UnlockWorkflow.
Subclasses are probably the way to go in Java, since the catch mechanism works on exception types, not an exception's fields.
Then, we would do the translation in this place (org.cryptomator.ui.unlock.UnlockInvalidMountPointController)
There was a problem hiding this comment.
Done. The only nuisance now is that the generic error dialog suggest to open the app preferences, although the misconfiguration should be resolved in the vault options.
Edit: Our comments overlapped, my "done" referred to only your first comment.
There was a problem hiding this comment.
We don't translate exception messages but only the UI.
Isn't this a bit of an edge-case, as here, the exception message will get displayed as part of the text in the "Unable to mount vault to custom path" dialog?
Either create some specific IllegalMountPointException subclasses
Ok, done.
There was a problem hiding this comment.
@overheadhunter this should be good to run the GitHub workflow again.
sschuberth
force-pushed
the
win-drive-occupied
branch
from
b564f7a
to
71ba73b
Compare
The error message was restored from 6395f17. Fixes cryptomator#2309.
sschuberth
force-pushed
the
win-drive-occupied
branch
from
71ba73b
to
173b1e8
Compare
There was a problem hiding this comment.
LGTM, thank you for your contribution!
@infeo can you have a look as well? Feel free to merge if you approve.
|
The CI build fails due to a CVE in Jackson. Shall I look into bumping the Jackson version as well? |
|
According to FasterXML/jackson-databind#3972, this is not considered a vulnerability but rather some "security researcher" wanting to distinguish himself by committing CVEs for normal and expected behavior of broadly used libs. Anyway, even if we consider this a vulnerability: Neither does Cryptomator pass user-defined data to the serializer, nor is it a centralized infrastructure that can be considered a target for a DOS attack. Therefore, please just add a rule for this specific CVE and library to the suppression.xml. Feel free to add a link to this comment in the description. There is no fix available anyway. 😅 |
Done. Please approve the workflow run once more. |
Fixes #2309.