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: Implement print intent #1038

Merged
merged 8 commits into from
Dec 8, 2023
Merged

feat: Implement print intent #1038

merged 8 commits into from
Dec 8, 2023

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Nov 27, 2023

  • AA - Interface permettant au cozy-app de demander une impression (cozy-intent.call(print) - la cozy-app fourni la base 64 du fichier à imprimer )
  • AA - Intégrer la librairie https://github.com/christopherdro/react-native-print
  • AA - Sauvegarde du fichier dans un dossier temporaire (voir OCR)
  • AA - Afficher un toast si erreur
  • AA - Avoir une méthode de type (isprintavailable)
  • Add tests

@acezard acezard changed the title feat: Implement base cozy-intent feat: Implement print inten Nov 27, 2023
@acezard acezard changed the title feat: Implement print inten feat: Implement print intent Nov 27, 2023
@acezard acezard marked this pull request as ready for review December 3, 2023 14:50

await RNPrint.print({ filePath: path })
await RNFS.writeFile(path, fileContent, 'base64')
await RNPrint.print({ filePath: path.replace('file://', '') })
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do a path.replace again ? It is already in getTemporaryPrintFilePath no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, need to remove it. With the prefix the print lib can't find the file. Maybe writeFile could not use it so it's clearer

Copy link
Contributor Author

@acezard acezard Dec 4, 2023

Choose a reason for hiding this comment

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

@zatteo to sum up: from what I understand I need the file prefix when writing the file, but when reading it with the print library, having that prefix will make the print software unable to read the file. I made it clearer by abstracting this file prefix in a "writeFile" local function

edit: after further test it seems I don't need the file prefix even when writing file. Do you remember why you had to use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

For backup feature, it seems that I remove 'file://' both for iOS and Android.

I do not know what it is kept on ocr feature / if it's needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For backup feature, it seems that I remove 'file://' both for iOS and Android.

I do not know what it is kept on ocr feature / if it's needed here.

for writing file, it seemed to work both with and without file prefix on Android. I just ignored it altogether in the most recent revision, not prepending it anywhere

@acezard acezard requested a review from zatteo December 4, 2023 15:34
@acezard acezard force-pushed the feat--implement-print-intent branch from 5497f6a to 0c7639b Compare December 4, 2023 15:45
Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

Nice PR, commits are well split so it was easy to read 👍

const date = format(new Date(), 'yyyyMMddHHmmssSSS')
const tempFileName = `temp_print_${date}.printfile`

const tempFileName = `temp_print_${date}.${fileExtension}`
Copy link
Member

Choose a reason for hiding this comment

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

I didn't print anything for a long time, but I remember that in certain scenario the printer would print the file name in the page footer. Is this still a thing for Android and iOS devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see it, and when saving as PDF Android discards the temp file name and creates its own filename from scratch. Though in any case, I make the name more end-user presentable if it's ever shown

}

const writeFile = async (filePath: string, content: string): Promise<void> => {
return await RNFS.writeFile(`${filePath}`, content, 'base64')
Copy link
Member

Choose a reason for hiding this comment

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

nit: we don't need this templated string anymore here.

Suggested change
return await RNFS.writeFile(`${filePath}`, content, 'base64')
return await RNFS.writeFile(filePath, content, 'base64')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

log.error(`Error while printing document: ${getErrorMessage(err)}`)
Toast.show({
type: 'error',
text1: t('error.unknown_error')
Copy link
Member

Choose a reason for hiding this comment

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

Should we be more specific in the toast message to state that this is about the print feature? Something like Failed to print, an error occured

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree ✅

@acezard acezard force-pushed the feat--implement-print-intent branch from 1de9f6b to 5b73d97 Compare December 8, 2023 09:53
@acezard acezard merged commit c56c77a into master Dec 8, 2023
1 check passed
@acezard acezard deleted the feat--implement-print-intent branch December 8, 2023 10:04
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.

3 participants