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

Use Leptonica API to access internals of Box #3851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stweil
Copy link
Member

@stweil stweil commented Jun 24, 2022

Signed-off-by: Stefan Weil [email protected]

@stweil
Copy link
Member Author

stweil commented Jun 24, 2022

See also #3673 (comment).

Please review carefully (the modified code is not covered by unittests, and I don't want to introduce new bugs).

@amitdo
Copy link
Collaborator

amitdo commented Jun 24, 2022

I reviewed this patch. I looks right, but too verbose :(

I approve it.

@egorpugin
Copy link
Contributor

egorpugin commented Jun 24, 2022

I think we need wrappers here.

auto box_get_geometry(Box *b) {
   struct coords {
      int32_t x;
      int32_t y;
      int32_t w;
      int32_t h;
   } c;
   boxGetGeometry(b, &c.x, &c.y, &c.w, &c.h);
   return c;
}
// and usage:
auto &&next = box_get_geometry(b);
next.x;
auto &&prev = box_get_geometry(b);
prev.w;
auto &&[x,y,w,h] = box_get_geometry(b);
// etc.
auto &&bb = box_get_geometry(b);
bb.x += xshift;
bb.y += yshift;
box_set_geometry(b, bb);

@amitdo
Copy link
Collaborator

amitdo commented Jun 26, 2022

Another option is to add

struct boxGeometry {
      int32_t x;
      int32_t y;
      int32_t w;
      int32_t h;
   };

and

struct boxGeometry boxGetGeometryAsStruct()

to Leptonica's public interface.

In C++17 code we can use structured binding (like in Egor's example).

@stweil
Copy link
Member Author

stweil commented Jun 26, 2022

So maybe we should postpone this change until a new major release which is using C++17 is made.

@egorpugin
Copy link
Contributor

Next major release can jump into C++20 already.

@amitdo
Copy link
Collaborator

amitdo commented Jun 26, 2022

@stweil, I don't understand your comment. We already require C++17 since 5.0.0.

@p12tic
Copy link
Contributor

p12tic commented Sep 20, 2022

Patch looks good to me.

@zdenop
Copy link
Contributor

zdenop commented Nov 24, 2022

Can we merge this or are we waiting for something?

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.

5 participants