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

feat: rename, deletion, and validation support #1833

Merged
merged 1 commit into from
Feb 15, 2023
Merged

feat: rename, deletion, and validation support #1833

merged 1 commit into from
Feb 15, 2023

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jan 25, 2023

Motivation

  • Support for sketch folder and code file name validation. Cached cloud sketches use the Create Editor's validation. The rest uses the spec.
  • Support for renaming and deleting local and cloud sketches.
  • Renamed remote to cloud sketch in the UI.

Change description

Other information

Closes #1599
Closes #1825
Closes #649
Closes #1847
Closes #1882
Ref #1826 (partially fixes)

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos changed the base branch from #1599 to main January 27, 2023 09:23
@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jan 27, 2023
@kittaakos kittaakos changed the title fix: local sketch deletion + rename feat: rename, deletion, and validation support Jan 31, 2023
@kittaakos
Copy link
Contributor Author

There is another bug with the server. When IDE2 tries to rename a non-existing sketch via the POST /create/v3/files/mv endpoint, IDE2 expects an HTTP 404 (Not found), but the server fails with HTTP 500. IDE2 has no chance to handle it.

@kittaakos kittaakos force-pushed the #1825 branch 3 times, most recently from 8804332 to 31cc604 Compare February 1, 2023 10:25
@kittaakos kittaakos added the type: imperfection Perceived defect in any part of project label Feb 1, 2023
@kittaakos
Copy link
Contributor Author

kittaakos commented Feb 1, 2023

bug38e0806 + b5be42a

must-set-focus-in-editor-after-rename.mp4

Upstream: eclipse-theia/theia#12139

@kittaakos
Copy link
Contributor Author

kittaakos commented Feb 1, 2023

When deleting the sketch, the window closes, but the sketch is not cleaned up from the filesystem. (On Windows)

Update: hacked in 743c9bf

@kittaakos
Copy link
Contributor Author

kittaakos commented Feb 1, 2023

when IDE2 prompts a save as to repair the sketch folder name, it should propose a valid name.

e4d12be

@kittaakos kittaakos force-pushed the #1825 branch 6 times, most recently from 5b7a045 to 0e6c19f Compare February 3, 2023 14:06
@kittaakos kittaakos marked this pull request as ready for review February 3, 2023 14:06
export function sketchNotFound(input: string): string {
return nls.localize(
'arduino/cloudSketch/notFound',
"Could not pull the cloud sketch '{0}'. It does not exist.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "cloud" be capitalized?

I'm not a fan of the recent trend I've observed of sometimes capitalizing "sketch" in sentences, since that is not a proper noun, but if "cloud" is short for the "Arduino Cloud" service (as opposed to being a reference to the general concept of "the cloud"), then this is a proper noun and so should be capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should "cloud" be capitalized?

I will leave this decision to you. Whichever you think is the best.

the recent trend I've observed of sometimes capitalizing "sketch" in sentences

I found the following translations from this PR where the "sketch" is capitalized in a sentence. (I hope I did not leave out anything):

These have not changed in this PR or recently:

'Pulling this Sketch from the Cloud will overwrite its local version. Are you sure you want to continue?'

'Public. Anyone with the link can view the Sketch.'

'This is a Public Sketch. Before pushing, make sure any sensitive information is defined in arduino_secrets.h files. You can make a Sketch private from the Share panel.'


These have not changed, and the sentence starts with "sketch":

"Sketch '{0}' must be verified before starting a debug session. Please verify the sketch and start debugging again. Do you want to verify the sketch now?",


These are command or menu labels, so capitalized "sketch" is expected:

label: nls.localize('arduino/sketch/new', 'New Sketch'),

label: nls.localize('arduino/sketch/showFolder', 'Show Sketch Folder'),

nls.localize('arduino/menu/sketch', 'Sketch')

name: nls.localize('arduino/sketch/sketch', 'Sketch'),

label: nls.localize('arduino/sketch/archiveSketch', 'Archive Sketch'),

label: nls.localize('arduino/cloudSketch/new', 'New Cloud Sketch'),

label={nls.localize('arduino/sketchbook/newSketch', 'New Sketch')}

title: nls.localize('arduino/cloud/shareSketch', 'Share Sketch'),


These are new and were requested in the design doc:

Let me know what to do. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave this decision to you. Whichever you think is the best.

I surveyed the Arduino Cloud-related content on arduino.cc and found that "Cloud" is capitalized in contexts equivalent to "Cloud sketch". So I think it should be capitalized.

the sentence starts with "sketch":

These are command or menu labels, so capitalized "sketch" is expected:

Yes, in cases where the work occurs at the start of a sentence or contexts where the standard for capitalization style is "title case", there is no question that "Sketch" is correct.

These are new and were requested in the design doc:

Then I guess I would have to request the input of the designers.

@91volt @gmarchiarduino please provide clear rules for when "sketch" should and should not be capitalized. As I've said several times in the past, it would be very helpful to have this sort of thing documented in a style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is something I cannot control, so I moved it to its dedicated task: #1884.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: Fixed by fef3b6a
Cloud sketch can be created with Windows-incompatible trailing .

Windows does not support folder names ending in `.`:

https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#:~:text=Do%20not%20end%20a%20file%20or%20directory%20name%20with%20a%20space%20or%20a%20period

Do not end a file or directory name with a space or a period. Although the underlying file system may support such names, the Windows shell and user interface does not.

When creating/renaming local sketches, Arduino IDE automatically removes a trailing . if the user adds one, but this is not done when creating/renaming Cloud sketches.

To reproduce

  1. Start the IDE build from this PR on a Windows machine.
  2. Select File > New Cloud Sketch from the Arduino IDE menus.
  3. Type EndsWithDot. in the sketch name field.
  4. Click the "OK" button.
    🐛 Multiple errors notifications and a spurious sketch name mismatch dialog appear:
    image
  5. Click the "OK" button on the dialog.
    🐛 The sketch fails to open.
  6. Open the EndsWithDot. sketch from the Cloud sketchbook.
    🐛 The sketch fails to open.

Arduino IDE version

bf6c814 (tester build for 9687fc6)

Operating system

Windows

Operating system version

11

@kittaakos
Copy link
Contributor Author

Great review!

Arduino IDE automatically removes a trailing . if the user adds one

I was surprised because there is no such logic in IDE2. It turned out, the OS is cutting this, not IDE2. Please take a look at the attached screencast. Electron's native file dialog does it on save:

os_trims_the_dot.mp4

When using the CLI:

c:\Users\kittaakos\Desktop\sketchbooks>arduino-cli.exe version
arduino-cli.exe  Version: 0.30.0 Commit: 83700ca2 Date: 2023-02-08T14:48:15Z

c:\Users\kittaakos\Desktop\sketchbooks>arduino-cli.exe sketch new endswithdot.
Sketch created in: c:\Users\kittaakos\Desktop\sketchbooks\endswithdot

Doing with mkdir from CMD.EXE:

c:\Users\kittaakos\Desktop\sketchbooks\test>mkdir test.

c:\Users\kittaakos\Desktop\sketchbooks\test>dir
 Volume in drive C has no label.
 Volume Serial Number is 04F3-13CC

 Directory of c:\Users\kittaakos\Desktop\sketchbooks\test

02/09/2023  10:16 AM    <DIR>          .
02/09/2023  10:16 AM    <DIR>          ..
02/09/2023  10:16 AM    <DIR>          test
               0 File(s)              0 bytes
               3 Dir(s)  215,877,074,944 bytes free

Closes #1599
Closes #1825
Closes #649
Closes #1847
Closes #1882

Co-authored-by: Akos Kitta <a.kitta@arduino.cc>
Co-authored-by: per1234 <accounts@perglass.com>

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Very excellent work. Thanks Akos!

@kittaakos kittaakos self-assigned this Feb 15, 2023
@kittaakos kittaakos merged commit d68bc4a into main Feb 15, 2023
@kittaakos kittaakos deleted the #1825 branch February 15, 2023 13:09
@per1234 per1234 added the topic: cloud Related to Arduino Cloud and cloud sketches label Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cloud Related to Arduino Cloud and cloud sketches topic: code Related to content of the project itself type: enhancement Proposed improvement type: imperfection Perceived defect in any part of project
Projects
None yet
2 participants