-
Notifications
You must be signed in to change notification settings - Fork 326
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
Change the default location for Enso projects #10318
Change the default location for Enso projects #10318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving changes to the PM shim used by GUI2's dev server
@@ -2791,6 +2792,17 @@ lazy val `benchmarks-common` = | |||
) | |||
.dependsOn(`polyglot-api`) | |||
|
|||
lazy val `desktop-environment` = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 3rd party dependencies. I like such lightweight modules.
return DirectoriesFactory.getInstance(); | ||
} | ||
|
||
public static String getOsName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have to define the getOsName()
method at all...
lib/java/desktop-environment/src/main/java/org/enso/desktopenvironment/Platform.java
Outdated
Show resolved
Hide resolved
import org.enso.desktopenvironment.directories.Directories; | ||
import org.enso.desktopenvironment.directories.DirectoriesFactory; | ||
|
||
public final class Platform { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum Platform {
LINUX, MAC, WINDOWS
}
might be better choice from an API perspective.
...-environment/src/main/java/org/enso/desktopenvironment/directories/DirectoriesException.java
Outdated
Show resolved
Hide resolved
@Override | ||
public Path getDocuments() throws DirectoriesException { | ||
try { | ||
var process = new ProcessBuilder(PROCESS_REG_QUERY).start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding a process execution would be nicer, but not even stack over flow has a suggestion how to do that nicely.
} | ||
|
||
private Path getXdgDocuments() throws IOException, InterruptedException { | ||
var process = new ProcessBuilder(PROCESS_XDG_DOCUMENTS).start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for using xdg-user-dir
I am looking forward for Enso to put the projects into the right directory on my Linux box!
|
||
import java.nio.file.Path; | ||
|
||
public interface Directories { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want other code outside of this module to implement this interface? If not, then prevent it:
- (in old Java days) I'd recommend
abstract class
with package private constructor - now there is also an option to defined
sealed
interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good old Java days 🤠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you deleting this file? Are the reasons for its existence obsolete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added in #4098 as a dummy Java file to be able to open the project manager in IVG.
lib/scala/project-manager/src/main/scala/org/enso/projectmanager/boot/configuration.scala
Show resolved
Hide resolved
try { | ||
return getXdgDocuments(); | ||
} catch (IOException | InterruptedException e) { | ||
return getUserHome().resolve("enso"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log a warning about a fallback?
try { | ||
return getUserHome().resolve(DOCUMENTS).toRealPath(); | ||
} catch (IOException e) { | ||
throw new IOException("Failed to resolve real MacOs documents path", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no fallback to whatever we have now?
|
||
return Path.of(stdoutParts[4].trim()); | ||
} catch (IOException e) { | ||
throw new IOException("Failed to run Windows registry query", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No fallback?
Pull Request Description
close #10240
Changelog:
desktop-environment
Java module to detect user environment configurationProjectsMigration
module containing the migration logic of the enso projects directorydesktopEnvironment
TS module to detect user environment configuration in theproject-manager-shim
project-manager-shim
with the new user projects directoryImportant Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.