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

fix: allow json and text fiels to be in preview file component #1646

Merged
merged 7 commits into from
Sep 24, 2020

Conversation

rafaelramalho19
Copy link
Contributor

@rafaelramalho19 rafaelramalho19 commented Sep 18, 2020

Closes #1642
Closes #1647
Important info: https://github.com/achingbrain/uint8arrays#tostringarray-encoding--utf8

Questions

  • Did ipfs.cat start returning uint8arrays recently?
  • Do we need to parse uint8arrays in another function?

src/files/file-preview/FilePreview.js Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

nit: it is not possible to select text because it tries to drag&drop entire preview.
I think we should disable d&d for text previews, as its more usefulf for people to be able to select part of text and copy it.

@rafaelramalho19
Copy link
Contributor Author

nit: it is not possible to select text because it tries to drag&drop entire preview.
I think we should disable d&d for text previews, as its more usefulf for people to be able to select part of text and copy it.

Done! Also added user-select: all, so if the user double clicks the text/json, it copies everything :)

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 thanks!

last nit: you switched to concat, which shows full file, but @jessicaschilling agreed we should be keeping first to avoid loading unbound text files into browser. Mind changing it back to first and if buf.length > 256000 show a footer at the end of preview informing user that preview was truncated?

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 something is still fishy: (1) clicking on "Load more" loaded entire thing instead of just one more chunk, and still displayes the footer. When i click on "Load more" again the footer disapears (2) footer is displayed for small text file, where is nothing more to load

Are you able to reproduce? (I'm on Firefox)

@achingbrain
Copy link
Member

Did ipfs.cat start returning uint8arrays recently?

Yes - in ipfs@0.50.x and ipfs-http-client@47.x.x. See: ipfs/js-ipfs#3220 and https://blog.ipfs.io/2020-09-14-js-ipfs-0-50/

@rafaelramalho19
Copy link
Contributor Author

@rafaelramalho19 something is still fishy: (1) clicking on "Load more" loaded entire thing instead of just one more chunk, and still displayes the footer. When i click on "Load more" again the footer disapears (2) footer is displayed for small text file, where is nothing more to load

Are you able to reproduce? (I'm on Firefox)

Yes @lidel , but the weird thing is that the buffer says that done: false and then on the second run just returns value: undefined, done: true

@lidel
Copy link
Member

lidel commented Sep 24, 2020

@rafaelramalho19 iterator docs suggest thats expected behavior?

value
The next value in the iteration sequence.

done
This is true if the last value in the sequence has already been consumed. If value is present alongside done, it is the iterator's return value.

[..] After a terminating value has been yielded additional calls to next() should simply continue to return {done: true}.

Some ideas:

  • Either account for that and execute additional next to see done: true
  • ..or, instead of relying on additional next call, you could check if currently read content length is equal to the file size reported by ipfs.files.stat in size (size of raw data without metadata)

rafaelramalho19 and others added 3 commits September 24, 2020 16:53
This means right click → save as
and "Download" buttons will result in file save dialog that defaults to
the name used in MFS
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@rafaelramalho19 works as expected and UX when opening huge text files is pretty nice now, thank you! 👌

Feel free to merge this and prepare a patch release, so we can include it in ipfs/ipfs-desktop#1638 🚢

src/files/file-preview/FilePreview.js Show resolved Hide resolved
@rafaelramalho19 rafaelramalho19 merged commit 24276f8 into master Sep 24, 2020
@rafaelramalho19 rafaelramalho19 deleted the fix/allow-json-and-text-in-preview branch September 24, 2020 21:11
@achingbrain
Copy link
Member

Some ideas:

  • Either account for that and execute additional next to see done: true
  • ..or, instead of relying on additional next call, you could check if currently read content length is equal to the file size reported by ipfs.files.stat in size (size of raw data without metadata)

Please do the first and not the second. If you do the second, the iterator will not know that it's not going to be invoked any more so will not perform any resource cleanup, tear down underlying streams etc, and could leak resources.

For example:

async function * gen () {
  yield 'hello'

  console.info('i will do cleanup now')
}

async function main () {
  console.info('using low-level API')
  const iter = gen()
  const val = await iter.next()
  console.info('val', val)
  console.info('not calling .next again..')

  console.info('---')

  console.info('using for...await of')
  for await (const val of gen()) {
    console.info('val', val)
  }
  console.info('all done')
}

main()

Running this file results in:

using low-level API
val { value: 'hello', done: false }
not calling .next again..
---
using for...await of
val hello
i will do cleanup now
all done

Note how when using the low-level .next() API, control is not passed back to the gen function after the yield statement.

@lidel
Copy link
Member

lidel commented Sep 28, 2020

Thanks @achingbrain, I've filled #1652 with an idea how to avoid the cleanup problem.


@rafaelramalho19 I don't believe this should block v2.11.2 release of webui – it has other urgent bugfixes that we should prioritise. Please tag v2.11.2 and bump webui to v2.11.2 in ipfs/ipfs-desktop#1638

(we will address #1652 in a later release of webui)

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.

Failed to view TXT file via WebUI Preview of .json files is broken
4 participants