-
Notifications
You must be signed in to change notification settings - Fork 702
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 GTK support #798
base: dev
Are you sure you want to change the base?
Add GTK support #798
Conversation
hey @mierzynskim you targetted the wrong branch, can you apply to 'dev' branch instead? |
@knocte yes, sure. Now I targeted dev branch |
@@ -0,0 +1,23 @@ | |||
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a file from your IDE, I think, please remove it from the PR
readme.md
Outdated
@@ -4,7 +4,8 @@ | |||
|
|||
![ZXing.Net.Mobile Logo](https://raw.github.com/Redth/ZXing.Net.Mobile/master/zxing.net.mobile_128x128.png) | |||
|
|||
ZXing.Net.Mobile is a C#/.NET library based on the open source Barcode Library: [ZXing (Zebra Crossing)](https://github.com/zxing/zxing), using the [ZXing.Net Port](https://github.com/micjahn/ZXing.Net). It works with Xamarin.iOS, Xamarin.Android, Xamarin.Mac, and Windows Phone. The goal of ZXing.Net.Mobile is to make scanning barcodes as effortless and painless as possible in your own applications. The new iOS7 AVCaptureSession barcode scanning is now also supported! | |||
|
|||
ZXing.Net.Mobile is a C#/.NET library based on the open source Barcode Library: [ZXing (Zebra Crossing)](https://github.com/zxing/zxing), using the [ZXing.Net Port](https://github.com/micjahn/ZXing.Net). It works with Xamarin.iOS, Xamarin.Android,Xamarin.Mac, GTK# and Windows Phone. The goal of ZXing.Net.Mobile is to make scanning barcodes as effortless and painless as possible in your own applications. The new iOS7 AVCaptureSession barcode scanning is now also supported! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing a space before "Xamarin.Mac"
this.Show(); | ||
this.DeleteEvent += new global::Gtk.DeleteEventHandler(this.OnDeleteEvent); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use spaces instead of tabs in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can change it because it's autogenerated file 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mkay
return null; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same nit here: spaces instead of tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Mmm, CI doesn't pass because of a weird NuGet error related to .sln or .csproj files:
Did you edit these files manually instead of letting VS change them? :) |
Hmm, I think CI doesn't have GTK installed... I thought it's shipped with Mono by default. I'll add it as external library |
Not sure adding it as external lib is the best approach. Maybe download the .msi file that installs these libs and install it non-interactively from the cake files? |
Hard to say. That's the approach XF chose https://github.com/xamarin/Xamarin.Forms/tree/master/Xamarin.Forms.Platform.GTK/Libs |
ah ok! if that's the case, you could do the same I guess, just grab the exact same binaries |
@Redth I think we have a final version now. I also tested it and it looks like it works fine. Please have a look when you have some time. |
Any news? Thanks |
@rblanca we have this merged in this fork in case you're interested: https://github.com/nblockchain/ZXing.Net.Xamarin (we also publish this fork on nuget) |
@Redth can you include this in 3.0? |
@knocte I'm super hesitant to add this into the main package, mostly because https://github.com/Redth/ZXing.Net.Mobile/pull/798/files?file-filters%5B%5D=.nuspec#diff-978f915984ac9573cd5e42dfc0550c3fR82-R83 The TFM of What we maybe could do is release a separate package of |
@knocte any interest in trying to rebase this PR's work on current master? |
What do you mean? what's the issue exactly? I've been using this code for a while now and works perfectly. The proof is this fork we publish on nuget: https://github.com/nblockchain/ZXing.Net.Xamarin and the app that consumes it: https://github.com/nblockchain/geewallet |
The problem is if I ship code that depends on GTK into an assembly which is in the net45 TFM, and someone tries to install and use the package in say a windows console app, they aren’t going to have a good time of it. Because there is no TFM that exists to pivot on GTK support directly, there’s limited options.
Basically 1 is less discoverable but much easier to implement than 2. I’m leaning towards option 1. |
But this is not supported by ZXing.Net.Mobile anyway AFAIU, why not just put a warning about this on the Readme? |
It's not supported today, but it could be one day. That alone may not be a good enough reason, but generally the practice we use at Microsoft is to use a separate package name for GTK stuff. Take I think the right thing to do here is the separate package (and one for the Forms implementation too). This means separate .csproj's which I don't love, but I think this is the right choice |
One day? AFAIU net45 is the old framework; and these days in the .NET community people are even advocating to stop supporting it because "legacy" and just support .NETCore, why would anyone go in the other direction? Anyway I understand the point of view (I use GTK myself with Xamarin.Forms) but it looks a bit ugly (because the other nonGTK platforms don't have a separate nuget package). |
Yes, net45 is old, however net5 is coming, the same problem will exist there. There will be no platform TFM for GTK pivoting. The point is more about unintentionally expressing support for a TFM which you do not actually support. Yes this is still true of a Yeah more nuget refs in a project stinks, but I definitely think the trade off is ok in this case. |
I have interest :) What I'm not sure is if I have time. Especially considering that even if I did the effort of rebasing it, I couldn't adopt the new 3.x verison of ZXing.Net.Mobile right away, because I just realised that you introduced Xamarin.Essentials as a dependency :( What was the motivation for that? It basically would break our setup because XamEssentials doesn't support macOS and Linux+GTK platforms. |
This PR will add rendering support for GTK #797. PR also includes example for both native GTK and XF backend.
Implementation of drawing an individual pixel may look like a little bit odd but I think that's the most readable version that can be achieved with GTK.
I wasn't sure how to tackle nuspec files for GTK but I assumed we can use the same convention as Xamarin Forms GTK backend and use
net45
as target framework (see here).