Add Windows support #121

jaimecbernardo · 2020-10-13T16:57:58Z

Adds support for react-native-windows.
Implements PDF printing.
Updates the example for react-native 0.63 and adds the windows platform to it.
Updates the docs.
Adds a test to the sample and uses it for automatic CI for Windows.

Adds support for printing PDFs in react-native-windows. Updates the documentation.

Also makes the reference to react-native-print relative, so that the current version of the code can be tested.

asklar · 2020-10-13T18:18:17Z

windows/RNPrint/RNPrint.h

+    const std::string Name = "RNPrint";
+
+    // Default name for the print job.
+    inline static const std::string defaultJobName = "Document";
+
+    // Name for the XAML Canvas to add preview folders to.
+    inline static const std::string RNPrintCanvasName = "RNPrintCanvas";


these can be constexpr instead

Will change them to char[] as well, so that they can be constexpr.

std::string can be constexpr with c++20, or you can use string_view in C++17. In general try avoiding C style character strings/arrays

asklar · 2020-10-13T18:19:04Z

windows/RNPrint/RNPrint.h

+    winrt::event_token printToken;
+
+    REACT_INIT(RNPrint_Init)
+      void RNPrint_Init(ReactContext const& context) noexcept


nit: spacing

windows/RNPrint/RNPrint.h

asklar · 2020-10-13T18:22:26Z

windows/RNPrint/RNPrint.h

+    }
+
+    // Generates a XAML page asynchronously.
+    concurrency::task<std::optional<xaml::UIElement> > GeneratePageAsync(PdfDocument pdfDocument, int pageNumber, PrintPageDescription pageDescription)


nit: you shouldn't need the space between > > anymore, I think? :)

muscle memory from times past :)

asklar · 2020-10-13T18:22:59Z

windows/RNPrint/RNPrint.h

+      float leftMargin = pageDescription.ImageableRect.X;
+      float topMargin = pageDescription.ImageableRect.Y;
+      float printableWidth = pageDescription.ImageableRect.Width;
+      float printableHeight = pageDescription.ImageableRect.Height;


make these const

asklar · 2020-10-13T18:24:26Z

windows/RNPrint/RNPrint.h

+            std::optional<xaml::UIElement> result = page;
+            return result;


return std::optionalxaml::UIElement(page);

or even more easily return page

asklar · 2020-10-13T18:26:44Z

windows/RNPrint/RNPrint.h

+            [=](auto const& sender, GetPreviewPageEventArgs const& args) -> void
+            {
+              auto printDocArg = sender.as<PrintDocument>();
+              int pageNumber = args.PageNumber();


asklar · 2020-10-13T18:28:57Z

windows/RNPrint/RNPrint.h

+
+      xaml::FrameworkElement root{ nullptr };
+
+      auto window = xaml::Window::Current();


this means this module won't work from xaml islands (which might be an ok compromise).
Another way to do this would be to fall back to use XamlUIService.XamlRoot if Window::Current is null

Will include the suggested workaround.
Is there a sample that uses xaml islands with react-native-windows that I could take a look into?

@jaimecbernardo thanks! we don't have any modules that do so today since they mostly seem to work and not require access to something like Window::Current, but we do have code in the react-native-windows repo today that deals with this. See usages of XamlUIService.XamlRoot() and search for "isxamlisland" or something like that, we have a helper in the repo for it

This is what I'm thinking of using :)
https://github.com/microsoft/react-native-windows/blob/df4cb47be1fcec7b5dc23ec032c0a75b19907d7f/vnext/Microsoft.ReactNative/Modules/LogBoxModule.cpp#L50-L52

asklar · 2020-10-13T18:30:29Z

windows/RNPrint/RNPrint.h

+    PrintDocument printDoc = nullptr;
+    IPrintDocumentSource printDocSource;
+    int numberOfPages = 0;
+    std::map<int, std::optional<xaml::UIElement>> pageCollection; // references to the xaml pages


it doesn't seem like you need this to be a map, you can just use std::vector

These can be generated out of order and a map also would support a future feature to allow page range printing.
I think a map works better here.
Using a map also follows the Windows Universal Samples code for the Printing API more closely.

agree that they can be generated out of order, but you are capturing the index i in the lambda, so maybe you can use that? not a big concern here, just looking to simplify since std::map is big and clunky

There's also the part of the when_all later on which wants to send it to the printer in the right order.
Since we are generating a XAML element for each member in the collection, the std::map overhead shouldn't have much impact in comparison.
The collection also isn't expected to end up having many members, since it's one per page.

asklar · 2020-10-13T18:40:03Z

windows/RNPrint/RNPrint.h

+      if (options.find("html") != options.end())
+        printOptions.html = options["html"].AsString();


avoid brace-less if statements whenever possible (here and other places)

asklar · 2020-10-13T18:43:26Z

windows/RNPrint/RNPrint.h

@@ -0,0 +1,402 @@
+#pragma once


this file is getting pretty large :) (github wouldn't show me the diff)
Can you break it up into a header and a source?

asklar · 2020-10-13T21:47:54Z

windows/RNPrint/RNPrint.h

+          {
+            cleanUp();
+            std::stringstream errorCode;
+            errorCode << "0x" << std::hex << action.ErrorCode() << std::endl;


undo the std::hex at the end?

Do you mean the std::endl?

I've used this code as a reference:
https://github.com/microsoft/react-native-windows-samples/blob/c1fe1ad0395d905c790b97edde76155efa9cced9/samples/NativeModuleSample/cppwinrt/windows/NativeModuleSample/AsyncMethodExamples.h#L68

But now that I think about it, that std::endl might not make sense.

I think it's a bug in the RNW code too :) basically std::hex changes state of the cout object, so any ints that come afterwards will be formatted as hex. AFAICT, endl only inserts a newline and doesn't reset the formatting options, so if someone prints a number to cout before anyone does << std::hex, it will show up in decimal, but if someone does it after this code runs, it will print as hex. Therefore it is unpredictable and we should reset to the default at the end of the line (<< std::dec << std::endl instead of std::endl)

Thank you. SGTM :)

jaimecbernardo · 2020-10-14T19:49:04Z

Hi @asklar.
Thank you for the feedback! 😄
I've added a couple of commits to address them.

stmoy · 2020-11-17T21:37:53Z

👋 Hi @christopherdro! @jaimecbernardo @asklar and myself are part of the team at Microsoft working on React Native for Windows. What do we need to do to get this PR pulled in?

christopherdro · 2020-11-17T22:22:32Z

Thank you for the PR everyone! I’ll have some free time next week during thanksgiving break to take a closer look.

asklar · 2020-11-17T22:37:47Z

windows/README.md

+On Windows, `react-native-print` needs an element in the visual tree to add the printable pages to.
+It will look for a XAML `Canvas` named `RNPrintCanvas` and use it.
+This needs to be added to the XAML tree of the screens where `react-native-print` is used.
+


@jonthysell I wonder if we should bake logic into autolinking so that modules can check that certain conditions are met before they get autolinked, e.g. that there is this element. Or at least give a chance for modules to print out warnings / instructions of this kind.
Filed microsoft/react-native-windows#6553

asklar

windows/RNPrint/RNPrint.cpp

Co-authored-by: Alexander Sklar <asklar@microsoft.com>

jaimecbernardo added 2 commits October 12, 2020 21:38

Add PDF printing support for Windows

40ffdde

Adds support for printing PDFs in react-native-windows. Updates the documentation.

Update example to react-native 0.63

e3e934d

Also makes the reference to react-native-print relative, so that the current version of the code can be tested.

jaimecbernardo mentioned this pull request Oct 13, 2020

Support react-native-print microsoft/react-native-windows#5314

Closed

2 tasks

asklar reviewed Oct 13, 2020

View reviewed changes

windows/RNPrint/RNPrint.h Outdated Show resolved Hide resolved

asklar reviewed Oct 13, 2020

View reviewed changes

Add react-native-windows to sample

7d2f36c

asklar reviewed Oct 13, 2020

View reviewed changes

jaimecbernardo added 3 commits October 13, 2020 17:32

Add windows test to example

22ce4b1

Add Windows CI as a Github action

f60ca89

Add Windows print canvas to windows/README.md

5078276

jaimecbernardo added 2 commits October 14, 2020 20:02

Fix Windows implementation code style

12b5c2a

Divide windows implementation into header and code

4498999

asklar reviewed Nov 17, 2020

View reviewed changes

asklar approved these changes Nov 17, 2020

View reviewed changes

asklar reviewed Nov 18, 2020

View reviewed changes

windows/RNPrint/RNPrint.cpp Outdated Show resolved Hide resolved

asklar approved these changes Nov 18, 2020

View reviewed changes

Fix code comment

89fc5b8

Co-authored-by: Alexander Sklar <asklar@microsoft.com>

christopherdro approved these changes Nov 24, 2020

View reviewed changes

christopherdro merged commit efa09ee into christopherdro:master Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows support #121

Add Windows support #121

jaimecbernardo commented Oct 13, 2020

asklar Oct 13, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 14, 2020 •

edited

Loading

asklar Oct 13, 2020

asklar Oct 13, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 13, 2020

asklar Oct 13, 2020

asklar Oct 13, 2020

asklar Oct 13, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 14, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 13, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 14, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 13, 2020

asklar Oct 13, 2020

asklar Oct 13, 2020

jaimecbernardo Oct 14, 2020

asklar Oct 14, 2020

jaimecbernardo Oct 14, 2020

jaimecbernardo commented Oct 14, 2020

stmoy commented Nov 17, 2020

christopherdro commented Nov 17, 2020

asklar Nov 17, 2020 •

edited

Loading

asklar left a comment


		xaml::FrameworkElement root{ nullptr };

		auto window = xaml::Window::Current();

		if (options.find("html") != options.end())
		printOptions.html = options["html"].AsString();

Add Windows support #121

Add Windows support #121

Conversation

jaimecbernardo commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asklar Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimecbernardo commented Oct 14, 2020

stmoy commented Nov 17, 2020

christopherdro commented Nov 17, 2020

asklar Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

asklar left a comment

Choose a reason for hiding this comment

asklar Oct 14, 2020 •

edited

Loading

asklar Nov 17, 2020 •

edited

Loading