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

Interface abstraction #28

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Craftplacer
Copy link
Collaborator

@Craftplacer Craftplacer commented Nov 7, 2024

Resolves #18
Resolves #23

@Craftplacer
Copy link
Collaborator Author

Craftplacer commented Nov 7, 2024

Key changes of this pull request so far:

  • SaneIsolate is split into implementation of Sane (SaneNative) and a class for managing the isolate itself (SaneIsolate)
    • SaneDevice with text info and methods were introduced, list of known device types is also available at SaneDeviceTypes
  • Sane is now an abstract interface with properly documented members, the implementation became SaneSync
    • SaneSync is used by SaneIsolate similar to before, sharing code.
  • Name lookup for SANE devices was replaced with just handing out the pointer address (which is only visible if you know the NativeSaneDevice type)
  • SaneNative uses device names for referencing devices (since we don't want to expose anything we don't want the developer to use themselves)
  • set_io_mode is not exposed anymore (we should handle it ourselves)
  • The isolate properly exits now

@Jupi007
Copy link
Collaborator

Jupi007 commented Nov 7, 2024

Why did you replaced SaneHandle with a simple int? I prefer the class encapsulation of that concept.

@Craftplacer Craftplacer force-pushed the interface-abstraction branch from 558a0d7 to bd6eb2b Compare November 10, 2024 14:44
@Craftplacer Craftplacer force-pushed the interface-abstraction branch 3 times, most recently from 72364d3 to 40d1495 Compare November 10, 2024 15:30
@Craftplacer Craftplacer marked this pull request as ready for review November 10, 2024 15:31
@Craftplacer Craftplacer force-pushed the interface-abstraction branch from 40d1495 to cd76013 Compare November 10, 2024 15:32
Copy link
Collaborator

@Jupi007 Jupi007 left a comment

Choose a reason for hiding this comment

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

Little nitpicks :)
Sorry for the delay, I was too busy...

Uint8List read({required int bufferSize}) {
_checkIfDisposed();

final handle = _handle ??= _open();
Copy link
Collaborator

@Jupi007 Jupi007 Nov 12, 2024

Choose a reason for hiding this comment

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

Maybe create a _handleOrOpen method/getter?

Copy link
Collaborator Author

@Craftplacer Craftplacer Nov 22, 2024

Choose a reason for hiding this comment

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

I disagree, because the behavior is already so apparent (_handle ??= _open() = "if handle is not null, return handle, otherwise open new handle"), it wouldn't bring much benefit other than it being an explicit method call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless you want to combine _open() and this behavior, then maybe we could do that, other than that, I don't see much reason. Happy to discuss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I totally forgotten your PR... too busy 😓

What I mean is to remove this line duplicated in almost each function:

- final handle = _handle ??= _open();
- dylib.sane_function(handle);
+ dylib.sane_function(_handleOrOpen);

The goal is just to make the code less verbose.

And yes, this could be merged with open, or inside a dedicated getter/method 🤔

packages/sane/lib/src/type_conversion.dart Outdated Show resolved Hide resolved
@Craftplacer Craftplacer requested a review from Jupi007 November 28, 2024 07:57
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.

Document code Proposal to encapsulate device access to a dedicated class
2 participants