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.

Sign up for GitHub

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

Jump to bottom

Interface abstraction #28

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

Interface abstraction #28

wants to merge 4 commits into from

Conversation

Craftplacer
Copy link
Collaborator

@Craftplacer Craftplacer commented Nov 7, 2024 •
edited
Loading

Resolves #18
Resolves #23

@Craftplacer
Copy link
Collaborator Author

Craftplacer commented Nov 7, 2024 •
edited
Loading

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 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
Jupi007
Jupi007 reviewed Nov 18, 2024
View reviewed changes
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...

packages/sane/lib/src/impl/sane_sync.dart
Uint8List read({required int bufferSize}) {
_checkIfDisposed();

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

@Jupi007 Jupi007 Nov 12, 2024 •
edited
Loading

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?

packages/sane/lib/src/type_conversion.dart
Comment on lines +207 to +210
@Deprecated('Use extension')
SANE_String_Const saneStringFromDartString(String string) {
return string.toNativeUtf8().cast<SANE_Char>();
return string.toSaneString();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be removed, we don't need backward compatibility yet. And anyway, this shouldn't be exported :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jupi007 Jupi007 Jupi007 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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