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

FileView #277

Closed
wants to merge 24 commits into from
Closed

FileView #277

wants to merge 24 commits into from

Conversation

MrStashley
Copy link

@MrStashley MrStashley commented Feb 7, 2024

Todo for now:

  • download should pop up file viewer window, get file/ folder from pop up, and download to that location on local
  • upload button that also pops up file viewer window

show download / upload progress on bottom status bar

  • search functionality

    • modal already gets search field

    • need to implement a searchdir / command on backend and going to need new mshell functionality

    • this should use the go filepath.walk to recursively search a dir and return all matching files

    • maybe not a v1 thing, but I feel like the search modal lends itself to more complex search terms

      • file extension search
      • show hidden files check mark
      • regex
      • recursive / not recursive
        any other fields that we might want to show for file searching?
    • click and drag functionality

    Should I break up this pr potentially in order to get file view in for testing? perhaps I could try to merge this after download / upload and then have search and click and drag be follow up prs?

Issues: seems like cwd doesn't work correctly on remote machines, need to debug


go func() {
var outputPos int64
StatDir(context.Background(), ids, path, func(statPk *packet.FileStatPacketType, done bool, err error) {
Copy link
Author

@MrStashley MrStashley Feb 8, 2024

Choose a reason for hiding this comment

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

callback paradigm, thoughts?
I thought this was a little less code than using a channel here
and I think it works just as well because the callback should be a closure on the data that we need like the context and the cmd object etc

right?

Copy link
Author

Choose a reason for hiding this comment

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

I guess the down side is the execution of the StatDir function stops while the callback is being executed
I think that is okay for this purpose because the callback is just gonna write to the pty and then return

but maybe a channel would make it faster

);
let curLineBytes = new Uint8Array(this.rawData.buffer, this.parsePos, i - this.parsePos);
let jsonStartIndex = curLineBytes.indexOf("{".charCodeAt(0));
let byteSize = curLineBytes.slice(jsonStartIndex, curLineBytes.length).length;
Copy link
Author

Choose a reason for hiding this comment

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

get size from raw byte array rather than later on from encoded rune string

return;
}
this.curDirectory = packet.path;
this.outputPos = packet.outputpos;
Copy link
Author

Choose a reason for hiding this comment

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

currently we store outputpos when we send commands, so that the new command can pick up where the old one left off.

This honestly sucks and is gonna bring about a lot of timing issues as well as a lot of extra code, but I wasn't sure any other way to do this
Is there a way to get the current output pos from a file?
I assume there are ways to get it with seek
I tried getting the file offset from the cirfile stat but that didn't work

need to make a note to dig deeper into this and try and find a low level solution that doesn't involve sending output pos across the wire

if len(pk.Args) > 0 {
path = pk.Args[0]
}
cmd, err := sstore.GetCmdByScreenId(ctx, pk.Kwargs["screenid"], pk.Kwargs["lineid"])
Copy link
Author

@MrStashley MrStashley Feb 14, 2024

Choose a reason for hiding this comment

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

currently this doesn't conform with the "outputpty" paradigm. should implement a version of this that outputs to a line if "outputpty" is not specified, because I think this might be useful to a user as well

@@ -1125,7 +1130,10 @@ func prettyPrintByteSize(size int64) string {
}

// this can only be called in a defer func, because recover() only works inside of a defe
func deferWriteCmdStatus(ctx context.Context, cmd *sstore.CmdType, startTime time.Time, exitSuccess bool, outputPos int64) {
func deferWriteCmdStatus(ctx context.Context, cmd *sstore.CmdType, startTime time.Time, exitSuccess bool, outputPos int64, outputPty bool) {
Copy link
Author

Choose a reason for hiding this comment

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

if outputPty is true, no need to close the cmd, so this function just returns

I'm not sure if that is clear to someone who is reading this for the first time. I maybe should change the name of this variable or add a comment

yarn.lock Outdated
@@ -6616,6 +6616,11 @@ parseurl@~1.3.2, parseurl@~1.3.3:
resolved "https://registry.yarnpkg.com/parseurl/-/parseurl-1.3.3.tgz#9da19e7bee8d12dff0513ed5b76957793bc2e8d4"
integrity sha512-CiyeOxFT/JZyN5m0z9PfXw4SCBJ6Sygz1Dpl0wqjlhDEGGBP1GnsUVEL0p63hoG1fcj3fHynXi9NYO4nWOL+qQ==

path-browserify@^1.0.1:
Copy link
Author

@MrStashley MrStashley Feb 14, 2024

Choose a reason for hiding this comment

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

Tried to get node.js "path" utility to work in frontend js, but there ended up being more webpack issues than I could debug. For now, path.join works, but path.relative invokes a "process" package that webpack doesn't resolve correctly. I will need to get a path utility a different way, implement my own, or get some help here

@MrStashley MrStashley changed the title FileVIew FileView Feb 20, 2024
}
}
sstore.MainBus.SendScreenUpdate(cmd.ScreenId, update)
update = &sstore.ModelUpdate{}
if destRemote == LocalRemote && sourceRemote == LocalRemote {
Copy link
Author

@MrStashley MrStashley Feb 22, 2024

Choose a reason for hiding this comment

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

I'm thinking of refactoring this so that all copy files to "copyfileRemoteToRemote" and go through the mshell

This might be slower but it will remove a ton of boilerplate and make the code a lot more readable and help avoid bugs
I think, since latency to the local wave shell is not currently a concern, it shouldn't impact speed significantly

@MrStashley MrStashley marked this pull request as ready for review February 25, 2024 01:19
<If condition={!file.isdir}>
<div className="download-button" onClick={(event) => downloadButtonClicked(event)}>
<i className="fa-sharp fa-solid fa-download"></i>
<input
Copy link
Author

Choose a reason for hiding this comment

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

Currently the file dialog on Mac says "upload". I couldn't find a way to change this online, does any one know how?
It would be nice to be able to set a custom text like "upload" / "download", or just "open"

@sawka
Copy link
Member

sawka commented Mar 19, 2024

merged main and fixed conflicts to try to keep this in sync so it doesn't drift too far.

@MrStashley MrStashley closed this Apr 17, 2024
@sawka sawka deleted the MrStashley-file-view branch September 18, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants