-
-
Notifications
You must be signed in to change notification settings - Fork 488
feat: introduced cloud states #1901
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
Conversation
4c07da1
to
23906d9
Compare
124f0c4
to
9dade8e
Compare
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:
- make sure your internet connection is ON
- sign in to cloud from the cloud sketchbook
- make sure you have pulled at least one sketch (e.g.:
sketch_A
) - turn OFF your internet connection
- open a sketch from the cloud sketchbook that you already had pulled (e.g.
sketch_A
) - a new sketch window opens showing the cloud sketch
- 🐛 in the left sidebar, the user image is broken (I guess we should have a default image to show when we can't show the actual user image)
- 🐛 the cloud sketchbook is empty
Here's a recording
Screen.Recording.2023年03月01日.at.15.51.36.mov
I'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:
I went a little further and turned back on the internet connection and this is the result:
another recording:
Screen.Recording.2023年03月01日.at.16.16.34.mov
As 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 1
confirm.push.mov
The second one doesn't ask for confirmation and doesn't open the sketch in a new window:
screen recording 2
push.no.confirm.mov
I'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
e36127a
to
c0c33b3
Compare
- 🐛 in the left sidebar, the user image is broken (I guess we should have a default image to show when we can't show the actual user image)
I added a patch and the default account codicon is visible when loading the avatar has failed.
d4a4e11
to
25665d8
Compare
I'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.
Thanks! I could reproduce it when creating a cloud copy without renaming the local sketch. The problem was in ncp
, so I switched to a new lib and added a few tests.
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:
- ✅ this is fixed (now it always asks for confirmation before pushing a sketch)
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.
The second one doesn't ask for confirmation and doesn't open the sketch in a new window: - ✅ this is fixed
🐛 in the left sidebar, the user image is broken (I guess we should have a default image to show when we can't show the actual user image)
- ❌ this is still happening
🐛 the cloud sketchbook is empty
Thanks for the review!
- ❌ this is still happening
🐛 the cloud sketchbook is empty
Clicking on the "Refresh" button will have the cloud sketchbook loaded, but the user image will still remain unavailable.
I expected it would be OK. What is your expectation?
91volt
commented
Mar 10, 2023
✅ this is fixed (now it always asks for confirmation before pushing a sketch)
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:
🐛 the cloud sketchbook is empty
Clicking on the "Refresh" button will have the cloud sketchbook loaded.
I expected it would be OK. What is your expectation?
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.
Clicking on the "Refresh" button will have the cloud sketchbook loaded, but the user image will still remain unavailable.
I expected it would be OK. What is your expectation?
About this, I agree with the last comment from @91volt ⬇️
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.
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.
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."
- ❌ this is still happening
🐛 the cloud sketchbook is empty
Thanks! I fixed it
1899.mp4
as it may cause confusion for the user since there is no existing sketch being overwritten.
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 arduino.cloud.push.warn
preference for this use case.
Update
I tame IDE2 to ignore the arduino.cloud.push.warn preference for this use case.
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
f507341
to
7dd7b03
Compare
Thanks to all of you for the excellent reviews.
Uh oh!
There was an error while loading. Please reload this page.
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