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

BitArray.h header no longer gets installed #361

Closed
berolinux opened this issue Jul 21, 2022 · 10 comments
Closed

BitArray.h header no longer gets installed #361

berolinux opened this issue Jul 21, 2022 · 10 comments

Comments

@berolinux
Copy link

1.4.0 removes the BitArray.h header from PUBLIC_HEADERS. Since BitMatrix::getRow still returns a BitArray, I'm thinking this may be a bug rather than an intentional change.

BitArray is used among others by LibreOffice.

https://github.com/LibreOffice/core/blob/master/cui/source/dialogs/QrCodeGenDialog.cxx#L30
https://github.com/LibreOffice/core/blob/master/cui/source/dialogs/QrCodeGenDialog.cxx#L82

@axxel
Copy link
Collaborator

axxel commented Jul 21, 2022

Sorry to hear this change is causing trouble. It was indeed a very deliberate decision, knowing the risk of missing some use case and potentially annoying someone. But I saw no practical way of figuring out beforehand who might be depending on what "internal" API. With knowingly public APIs I'm trying hard to provide a smooth transition (see 1.3 -> 1.4 -> 2.0).

Since this particular use case in LibreOffice does not make much sense from neither a performance nor readability perspective, I'd like to propose to change it there. Please have a look at ToSVG and either copy that approach (matrix.get(x, y)) or maybe simply use ZXing::ToSVG as is (but that was only added in 1.4).

That said, I agree that it makes no sense to have that Matrix::getRow method and then not include BitArray.h. I should have removed/deprecated that method as well.

See also the second half of this comment which was a reply to @a17r who probably ran into the same issue?

@axxel
Copy link
Collaborator

axxel commented Aug 3, 2022

Are any of you (@berolinux or @a17r) in the position to provide a PR for libreoffice that does away with the use of Matrix::getRow? In 2.0 this method will be gone for good.

@axxel
Copy link
Collaborator

axxel commented Aug 18, 2022

@cloph I saw this comment of yours and wondered whether you would be willing feed a 3-line patch into the Gerrit machinery to remove the use of the deprecated Matrix::getRow call (see above).

@bwarden
Copy link

bwarden commented Dec 7, 2022

If it helps, here's the patch I created for Clear Linux:
https://github.com/clearlinux-pkgs/libreoffice/blob/main/0004-Remove-dependency-on-BitArray.h-from-zxing-1.2.0.patch

@axxel
Copy link
Collaborator

axxel commented Dec 8, 2022

@bwarden thanks a lot for linking it here. If only someone found the time to feed this patch into the Gerrit issue/PR infrastructure they use. I don't have the time to do that.

@reneengelhard
Copy link

I could theoretically do this but this is no real help - actually @bwarden should do it anyway given it's his code and LO probably needs his license statement anyway (cf. https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch#Add_yourself_to_the_contributor_list)

@axxel
Copy link
Collaborator

axxel commented Dec 29, 2022

@reneengelhard please go ahead and submit something. If you don't want to infringe on @bwarden s copyright, just have a look at the above mentioned ToSVG code from zxing-cpp itself as a template to remove the BitArray.h dependency. For half a year now, I'm hoping someone with a working gerrit setup would be so kind to send in those 3 lines. This keeps coming up again and again.

@reneengelhard
Copy link

reneengelhard commented Dec 30, 2022

https://gerrit.libreoffice.org/c/core/+/144874, with proper --author= for git

tdf-gerrit pushed a commit to LibreOffice/core that referenced this issue Dec 30, 2022
In zxing-1.4.0, numerous headers are no longer public. Rework the
ConvertToSVGFormat method so it uses bitmatrix.get instead of
bitmatrix.getRow, similar to the ToSVG method in zxing itself.

See zxing-cpp/zxing-cpp#361

Change-Id: Ie25eb8f782e8799fbd57c24ef79bba92acf0f9ff
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/144874
Tested-by: René Engelhard <rene@debian.org>
Reviewed-by: René Engelhard <rene@debian.org>
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
tdf-gerrit pushed a commit to LibreOffice/core that referenced this issue Dec 30, 2022
In zxing-1.4.0, numerous headers are no longer public. Rework the
ConvertToSVGFormat method so it uses bitmatrix.get instead of
bitmatrix.getRow, similar to the ToSVG method in zxing itself.

See zxing-cpp/zxing-cpp#361

Change-Id: Ie25eb8f782e8799fbd57c24ef79bba92acf0f9ff
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/144895
Tested-by: René Engelhard <rene@debian.org>
Reviewed-by: René Engelhard <rene@debian.org>
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
@axxel
Copy link
Collaborator

axxel commented Jan 1, 2023

Awesome! Who could have thought that this would get all the way down into debian testing within just 2 days?!? :)

If anyone listing here has the means to test this easily, I'd be interested to know of the current HEAD (which will be 2.0 very shortly) works with libreoffice as is. If not, now I still have the time to fix any upcoming issues.

@axxel axxel closed this as completed Jan 1, 2023
@reneengelhard
Copy link

If anyone listing here has the means to test this easily, I'd be interested to know of the current HEAD (which will be 2.0 very shortly) works with libreoffice as is. If not, now I still have the time to fix any upcoming issues.

seems so, at least buillds and links after nudiging LO in finding it in /usr/local after removing Debians packages

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

No branches or pull requests

4 participants