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

Allow Save As and breadcrumb navigation #2060

Merged
merged 20 commits into from
Sep 25, 2024
Merged

Allow Save As and breadcrumb navigation #2060

merged 20 commits into from
Sep 25, 2024

Conversation

sebjulliand
Copy link
Collaborator

Changes

Resolves #2004

This PR completes the QSYS and IFS FileSystemProviders implementation. By doing so, it allows for Save As and the editor breadcrumb navigation to work for members and streamfiles.

The stat method had to return an actual result, createDirectory and readDirectory had to be implemented, writeFile had to be modified because Save As first creates an empty file then writes into it.

Save As

Streamfiles and members can now be Saved As (using ctrl+shift+s or the menu action).

For streamfiles, it works like an usual unix file system. browsing the IFS works from there.
image

If the target path doesn't exist, it will be created.
image

Overwriting an existing file asks for a confirmation as expected.
image

For QSYS members, it works the same (navigation, creation of target path, overwriting). But since it's a QSYS path, the path must always be in the form of /LIBRARY/FILE/MEMBER.TYPE. The library and/or file will be created if needed when saving as.

Breadcrumb navigation

Clicking on the part of a path in an editor with a member or streamfile opened will allow to navigate to another location to open another member/stream.
image
image

How to test this PR

  • Open a streamfile or a member
  • Try navigating from the breadcrumbs
  • Try to Save As
    • As a new file in an existing directory/source physical file
    • As a new file in a non existing directory/library/source physical file
    • As an existing file (to overwrite it)

Checklist

  • have tested my change
  • Remove any/all console.logs I added

@sebjulliand sebjulliand added the enhancement New feature or request label May 16, 2024
@sebjulliand sebjulliand requested review from worksofliam and a team May 16, 2024 20:47
@sebjulliand sebjulliand self-assigned this May 16, 2024
@chrjorgensen
Copy link
Collaborator

@sebjulliand AMAZING - AND I HAVEN'T EVEN TESTED IT YET! YOU'RE MY HERO!! 😍

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand A quick run of this PR shows some issues:

  1. I created a library CODEFORI and sourcefile TEST and member MEMBER and added some text. Save went fine, but Save as... did not: After entering new membername MEMBER2, I get this:
    billede
    But this is the only member:
    billede

  2. I created a directory /tmp/a and a streamfile testing.rpgle, entered some text and saved. No issue.
    billede
    But Save as... just shows the directory name:
    billede
    When I enter a new name for the streamfile, it always shows this:
    billede
    But it doesn't exist - and OK gives this error:
    billede

* @param timestamp an attr timestamp string
* @returns a Date object
*/
export function parseAttrDate(timestamp:string){
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sebjulliand This code is worrying me a bit - you get the attributes using the PASE command attr, which can return IBM i object attributes, but it writes the dates in text, which you then have to parse.

The timestamps output from attr are dependent on the locale, which may vary from system to system - or even user by user.

Wouldn't it be better to get the attributes for an IFS object using the SQL function IFS_OBJECT_STATISTICS? It can return the same information and returns the timestamps in ISO format - or as EPOCH, if it is specified in the SQL statement. Like we do with member lists...

Then this parse function is irrelevant - and the month name at the top etc.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I tried (hard) to use IFS_OBJECT_STATISTICS, but it doesn't give any information if you don't have enough authority to do so. Every column will be null in this case. For example, there are some files on PUB400 that I can access in read-only mode. For those, IFS_OBJECT_STATISTICS will not give me any information.

I tried on a few different LPARS and even when I change LC_TIME before calling attr, it still returns the dates using the en_US locale.

So for now, I'll take the risk of using attr and parse the dates empirically. 😅

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen I think I finally managed to fix Save As for your use case.
A file name ending with . is not valid when using Save As (VSCode doesn't allow it, there is no way around this). But, if you want to save as a member with no extension, just don't put an extension in the name; QSYSFS will now handle it correctly.

I had to change a bit parserMemberPath to do so, but it's for the best (hopefully).
You can give it another go whenever you want. Thanks!

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand The use case with no member extension does work now. 👍

Only issue is that we require the "." when creating the member but require no "." when Save as... not consistent, but the opposite... but this is due to Microsoft not allowing file/directory names ending in "." in Windows where all other platforms allow it... 😠

I also found that not having /QOpenSys/pkgs/bin in the PATH results in Save as reporting
billede
Adding the yum package directory to the path fixed the problem.

I will do some more testing... 😃

@chrjorgensen
Copy link
Collaborator

chrjorgensen commented May 23, 2024

There are still some issue with Save as reporting the error:
billede

When I as the first thing open a member for browse and choose Save as, I get the error. Open for edit does not show this error, and after open for edit, I can open for browse and use Save as... sometimes, because often it accepts the save but does not actaully save...

Signed-off-by: Seb Julliand <[email protected]>
@sebjulliand
Copy link
Collaborator Author

@chrjorgensen give it another go. I removed the stat cache...hopefully the performance overhead won't be terrible.

Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand Quick review while watching Le Mans... 😄

Works alright when member is opened for edit - when opened for browse, the error This folder cannot be used as destination. Please choose another folder. is shown. If this cannot be avoided, maybe the Save As should reject the operation?

There is also an issue with member names with variants - I can't even open these members, even after merging master branch into the PR branch...? Can you make it work with variants?

Ideally the text of the new members should be set to the same as the original member - but this could be implemented in a future change.

@worksofliam
Copy link
Contributor

@sebjulliand What's the status here? Does it need a lil' TLC or can I review?

@sebjulliand
Copy link
Collaborator Author

@sebjulliand What's the status here? Does it need a lil' TLC or can I review?

@worksofliam I need to make some changes...this week of the week after 🥲

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen , it's back for review, finally 😅

Works alright when member is opened for edit - when opened for browse, the error This folder cannot be used as destination. Please choose another folder. is shown.

I could get Save As to work for members/streamfiles opened in read-only mode. The only limitation is that the saved as member/file will still be opened in read-only because its URI share the same fragment as its "parent". I can't overcome this until we change the way our file systems handle the read-only flag (something I'll keep in the back of my head 😄).

There is also an issue with member names with variants - I can't even open these members, even after merging master branch into the PR branch...? Can you make it work with variants

Done! Variant characters needed to be converted to their CSSID 37 counterparts before being fed to the attr command. It works well now. Even creating a source file on the fly with variants!

Ideally the text of the new members should be set to the same as the original member - but this could be implemented in a future change.

Sadly, I tried to find a way but since we cannot know which file is being saved as by the time the target is being created and written, we can't copy the description 😑

Copy link
Collaborator

@julesyan julesyan left a comment

Choose a reason for hiding this comment

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

I like it a lot. Code looks good to me. Tested locally with a variety of cases.

@sebjulliand
Copy link
Collaborator Author

Thanks a lot for looking into this @julesyan ! Let's merge 😄

@sebjulliand sebjulliand merged commit 59b85fd into master Sep 25, 2024
1 check passed
@sebjulliand sebjulliand deleted the feature/saveAs branch September 25, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use "Save As..." to create new member or stream file
4 participants