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

Notification about the project rename action #7613

Merged

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Aug 18, 2023

Pull Request Description

close #7604

After moving the rename action to the dashboard, IDE is unaware of the new project name. PR implements a new refactoring/projectRenamed notification that is sent from the server to clients and informs them about the changed project name.

Important Notes

enso-7613.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 self-assigned this Aug 18, 2023
@4e6
Copy link
Contributor Author

4e6 commented Aug 18, 2023

2023-08-18-221510_535x83_scrot

@farmaazon @MichaelMauderer how do I update the label with the project name? In this PR I implemented the update of the project properties, but it does not change the displayed text.

fn rename_project_on_notification(
project_renamed: language_server::types::ProjectRenamed,
properties: Rc<RefCell<Properties>>,
execution_contexts: Rc<ExecutionContextsRegistry>,
) {
let mut properties = properties.borrow_mut();
properties.displayed_name = project_renamed.new_name.into();
properties.project_name.project = project_renamed.new_normalized_name.clone().into();
execution_contexts
.rename_project(project_renamed.old_normalized_name, project_renamed.new_normalized_name);
}

@4e6 4e6 marked this pull request as ready for review August 21, 2023 13:50
@vitvakatu
Copy link
Contributor

@farmaazon @MichaelMauderer how do I update the label with the project name? In this PR I implemented the update of the project properties, but it does not change the displayed text.

We only read these properties once. Should be relatively easy change, I can do it in this branch later today if you want.

@4e6
Copy link
Contributor Author

4e6 commented Aug 21, 2023

@vitvakatu thank you, I'd appreciate your help. Or you can leave me some pointers on where to start, or what files need to be changed. Whatever is easier for you

@vitvakatu
Copy link
Contributor

@4e6 done, but I can't test it locally because I can't build the engine from the source. Please try it and tell me if it does not work. Also, I removed some old code from times the IDE was renaming on its own.

@4e6
Copy link
Contributor Author

4e6 commented Aug 21, 2023

@vitvakatu thank you! The project name is updated now. I added a screencast to the description

@@ -750,33 +774,6 @@ impl model::project::API for Project {
.boxed_local()
}

fn rename_project(&self, name: String) -> BoxFuture<FallibleResult> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean there is no way to rename the project while the project is open in the IDE? I tried clicking the project name on Friday and I was surprised there was no response. I guess that is intentional then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it is intentional. In the new design, you have to use the dashboard for renaming.

@vitvakatu
Copy link
Contributor

@Frizi or @MichaelMauderer please review, as I did some code changes myself.

Copy link
Contributor

@MichaelMauderer MichaelMauderer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed only rust files.

let mut properties = properties.borrow_mut();
properties.displayed_name = project_renamed.new_name.into();
properties.project_name.project = project_renamed.new_normalized_name.clone().into();
execution_contexts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is most likely fine currently, but it might be prudent to explicitly limit the mutable borrow doing another function call to avoid an accidental double borrow somewhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4e6 FYI, you can either add drop(properties) after the last line that uses them or surround properties-related code with curly braces to do that.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Aug 22, 2023
@mergify mergify bot merged commit 5dc2c4c into develop Aug 22, 2023
@mergify mergify bot deleted the wip/db/7604-notification-about-the-project-rename-action branch August 22, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification about the project rename action
5 participants