-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
feat: introduced cloud states #1901
Conversation
27be212
to
512dad5
Compare
arduino-ide-extension/src/browser/theia/core/window-title-updater.ts
Outdated
Show resolved
Hide resolved
I've stumbled in an edge case that resulted in having a broken cloud user icon and an empty cloud sketchbook. Steps to reproduce:
Here's a recordingScreen.Recording.2023-03-01.at.15.51.36.movI'm not sure what the expected behaviour should be here. If possible I guess we should load the cloud sketchbook in the same state as it is in the current window, but I'm not sure we can do that. @kittaakos thoughts? UPDATE: another recording:Screen.Recording.2023-03-01.at.16.16.34.movAs you can see, when the connection comes back, the situation doesn't change. Clicking on the "Refresh" button will have the cloud sketchbook loaded, but the user image will still remain unavailable. |
Another thing I stumbled into, but I'm not sure it's an unexpected behaviour or if I am missing something about the expected behaviour: In the following 2 screen recordings, I'm pushing sketches from my local sketchbook to the cloud sketchbook. The first one results in a different behaviour from the the second one, asking for user's confirmation before pushing the sketch, and after that opening the pushed sketch in a new window. screen recording 1confirm.push.movThe second one doesn't ask for confirmation and doesn't open the sketch in a new window: screen recording 2push.no.confirm.movI'm not sure about the confirmation message, but in the second recording I think it's expected to open the pushed sketch in a new window. |
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.
Thank you @kittaakos for this! Overall quality of the code is outstanding, as always.
I just left some of comments that in my opinion could improve the readability. Let me know what you think about them. I didn't request changes because they're not blocking.
Apart from those, I also left a couple of comments about unexpected behaviour.
arduino-ide-extension/src/browser/widgets/cloud-sketchbook/cloud-sketchbook-tree.ts
Show resolved
Hide resolved
arduino-ide-extension/src/browser/theia/messages/notifications-manager.ts
Show resolved
Hide resolved
arduino-ide-extension/src/browser/contributions/new-cloud-sketch.ts
Outdated
Show resolved
Hide resolved
arduino-ide-extension/src/browser/widgets/sketchbook/sketchbook-tree-widget.tsx
Outdated
Show resolved
Hide resolved
arduino-ide-extension/src/browser/widgets/sketchbook/sketchbook-tree-widget.tsx
Outdated
Show resolved
Hide resolved
I added a patch and the default account codicon is visible when loading the avatar has failed. |
d4a4e11
to
25665d8
Compare
Thanks! I could reproduce it when creating a cloud copy without renaming the local sketch. The problem was in Update: Cross-link from the Arduino forum: https://forum.arduino.cc/t/new-ide-inst-saving-files-correctly/1122494/9 |
@91volt, can you please help with the review? Thanks! |
Thank you @kittaakos for addressing my comments. I performed another run of tests and these are the results:
|
Thanks for the review!
I expected it would be OK. What is your expectation? |
I suggest removing the confirmation prompt when copying a sketch to the cloud, as it may cause confusion for the user since there is no existing sketch being overwritten. Additionally, both options currently result in the same outcome. About this:
Ideally, this glitch wouldn't exist at all, or the sketchbook would be refreshed automatically in such cases. If fixing it now is time-consuming, we could consider publishing a separate fix for this bug since it is not critical for the general purpose of this feature in my opinion. |
About this, I agree with the last comment from @91volt ⬇️
|
I too was thinking it would be a good idea to remove it. In the end, it's not possible to push a local sketch if it already exists in the cloud, therefore it doesn't make much sense to warn the user saying that "Pushing this Sketch will overwrite its Cloud version." |
Thanks! I fixed it 1899.mp4
Technically, there is. There is no such thing as create a sketch with these files endpoint. IDE2 has to create a default one and push the local files, so it's not an atomic operation. I tame IDE2 to ignore the Update
It's done ✅ |
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.
🟢 Green light for me!
Thank you Akos!
arduino-ide-extension/src/browser/theia/core/connection-status-service.ts
Outdated
Show resolved
Hide resolved
Closes #1879 Closes #1876 Closes #1899 Closes #1878 Signed-off-by: Akos Kitta <[email protected]>
Thanks to all of you for the excellent reviews. |
Motivation
Please reference the internal slides regarding the expected behavior.
editor_toolbar.mp4
offline.mp4
copy-to-cloud.mp4
cloud_prefix.mp4
Change description
[Cloud]
prefix in the window title,Other information
Closes #1879
Closes #1876
Closes #1899
Closes #1878
This PR includes 3f5eddb from #1910.Reviewer checklist