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

Add storage API to open a file for appending, without truncating it's content first #5176

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

mbaklor
Copy link
Contributor

@mbaklor mbaklor commented Sep 30, 2024

Implemented in both memory and desktop, need to implement for mobile still, setting this as a draft for now, I don't know who can do mobile and how but for now this is what I could pull off.

Description:

Needed an option to add to a file in the storage without truncating it first, specifically to write long lasting log files.

Fixes #(issue)

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: line.

implemented in both memory and desktop, need to implement for mobile
still
because I forgot how io reader works
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Clear and clean, thanks for looking at this - a good addition.

I think it will need the mobile code added to land though.
Would tips on how to progress be sufficient, or are you handing this on to someone else?

@mbaklor
Copy link
Contributor Author

mbaklor commented Oct 17, 2024

Thank you that was super encouraging and got me the drive to try the mobile code myself!

Let me know if this mobile code makes sense to you, and if someone with an android or ios setup - well - set up could check that this code works I'd super appreciate it as well!

@mbaklor mbaklor marked this pull request as ready for review October 17, 2024 22:34
@andydotxyz
Copy link
Member

Looking great. The code compiles and runs on my iOS test devices.
But there is no test for the file repository on mobile.
Is there a snippet of code that could be run, or an example we could drop into fyne_demo that would verify?

Thanks!

@mbaklor
Copy link
Contributor Author

mbaklor commented Oct 23, 2024

I have discovered in writing this test snippet that I in fact forgot to implement the last part of the puzzle, the storage interface itself. Fixed and pushed, and here's the snippet I used to check it on my PC

func main() {
	app := app.NewWithID("io.fyne.appendtest")
	win := app.NewWindow("Append Test")
	win.Resize(fyne.NewSize(400, 300))

	app.Storage().Create("test.txt")

	btnAppend := widget.NewButton("append to file", func() {
		doc, err := app.Storage().Append("test.txt")
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		_, err = doc.Write([]byte("test line\n"))
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		err = doc.Close()
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		dialog.ShowInformation("Success", "Wrote to file", win)
	})
	btnRead := widget.NewButton("read file", func() {
		doc, err := app.Storage().Open("test.txt")
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		b := make([]byte, 64)
		_, err = doc.Read(b)
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		dialog.ShowInformation("Success", "Successfully read from file\n\ncontents:\n"+string(b), win)
		println(string(b))
	})

	content := container.NewGridWithRows(2, btnAppend, btnRead)
	win.SetContent(container.NewCenter(content))
	win.ShowAndRun()
}

storage.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 26, 2024

Coverage Status

coverage: 59.969% (+0.008%) from 59.961%
when pulling 504b502 on mbaklor:storage-append
into de361f6 on fyne-io:develop.

@mbaklor
Copy link
Contributor Author

mbaklor commented Oct 26, 2024

the new "it works on my system" with the storage interface reverted to how it was is:

func main() {
	app := app.NewWithID("io.fyne.appendtest")
	win := app.NewWindow("Append Test")
	win.Resize(fyne.NewSize(400, 300))
	root := app.Storage().RootURI()
	uri, err := storage.Child(root, "test.txt")
	if err != nil {
		dialog.ShowError(err, win)
		return
	}

	btnAppend := widget.NewButton("append to file", func() {
		doc, err := storage.Appender(uri)
		defer doc.Close()
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		_, err = doc.Write([]byte("test line\n"))
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		dialog.ShowInformation("Success", "Wrote to file", win)
	})
	btnRead := widget.NewButton("read file", func() {
		doc, err := storage.Reader(uri)
		defer doc.Close()
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		b := make([]byte, 64)
		_, err = doc.Read(b)
		if err != nil {
			dialog.ShowError(err, win)
			return
		}
		dialog.ShowInformation("Success", "Successfully read from file\n\ncontents:\n"+string(b), win)
		println(string(b))
	})

	content := container.NewGridWithRows(2, btnAppend, btnRead)
	win.SetContent(container.NewCenter(content))
	win.ShowAndRun()
}

return fileWriterForURI(u, true)
}

// TODO: need someone who understands native code to write this, I'm not sure
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this TODO could go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OOPS! deleted

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I think there is still a breaking change in the Repository interface API, but we have discussed how to resolve it. I left notes here as well which I hope help.

// file if it exists
//
// Since: 2.6
Appender(u fyne.URI) (fyne.URIWriteCloser, error)
Copy link
Member

Choose a reason for hiding this comment

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

This is the remaining breaking change where a new (optional) interface will be needed, perhaps AppendableRepository?

storage/uri.go Outdated
return nil, err
}

wrepo, ok := repo.(repository.WritableRepository)
Copy link
Member

Choose a reason for hiding this comment

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

Corresponding to the note above this should be a check for supporting the Appender

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.

4 participants