Skip to content

feat: Improve onyxsdk-pen integration #1452

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

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

Conversation

zinstack625
Copy link

Updated onyxsdk dependencies

Makes stuff work on some newer devices. Don't really know why, but before that, stylus input resulted in full refreshes, as if there were no support for that altogether. This was observed both on Play Store and on local builds. May, but should not break other devices support

Reworked OnyxsdkPenArea.kt

  • Removed explicit drawing
    Much of the stuff done in drawPreview had absolutely no effects. Whole magic seems to happen in TouchHelper. Removing that manual drawing, again, may break stuff on other devices, but most of the logic remained nonetheless. In case of breakages, should be trivial to bring that stuff back, or, if my intuition is correct, further simplify this class. Would really like to hear some feedback on that removed code if possible
  • Established message passing between OnyxsdkPenArea and Dart
    In order to modify state of the PlatformView, message passing was required. It is used to update stroke parameters from Flutter UI toolbar to the PlatformView. I see the use as "refreshing" the creationParams. Might be useful in the future, both in of itself and as a precedent for other usecases
  • Mapped used tools to those in onyxsdk-pen
    This in effect resulted in directly-refreshed preview looking very similar to the underlying image. Most of the tools are practically indistinguishable and are only insignificantly changed in shape (underlying image is less detailed) and, in some cases significantly in color during full refresh (seen on attached video). Exception being the pencil (seen on attached video), but it's still pretty close. Don't think it can get closer without some legally questionable voodoo magic with onyxsdk

Dart side

Most are due to preparation of tool parameters to Kotlin side, some are due to canvas being unaware of the tool being used. I tried to make significant changes as close to the actual use as possible, but may have introduced a couple of unnecessary abstractions and translations along the way. Could possibly be done better, but I'm inexperienced in Flutter and Dart. Hopefully the changes are not disruptive

Seems to be doing good on Note Air 4C, but would be good if someone could test on other devices. This does not seem to introduce any slowdowns, does not touch any significant logic and does not introduce any UI changes

20250320.webm

Great project BTW

Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 47.29%. Comparing base (7d60459) to head (a9c8748).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/components/canvas/canvas.dart 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1452      +/-   ##
==========================================
+ Coverage   47.15%   47.29%   +0.13%     
==========================================
  Files         114      114              
  Lines        9002     9027      +25     
==========================================
+ Hits         4245     4269      +24     
- Misses       4757     4758       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Fe-Ti
Copy link

Fe-Ti commented Mar 20, 2025

Hi,

On my NoteAir 2 there is a huge lag (approx. 30s) after I lift my pen and before the actual stroke is rendered from scribble.

I tried to fix this issue and came up with this solution (same for eraser):

        override fun onBeginRawDrawing(b: Boolean, touchPoint: TouchPoint) {
            // begin of stylus data
            touchHelper.setRawDrawingRenderEnabled(true)
            reset()
        }

        override fun onEndRawDrawing(b: Boolean, touchPoint: TouchPoint) {
            // end of stylus data
            touchHelper.setRawDrawingRenderEnabled(false)
            reset()
            scheduleRefresh()
        }

With such toggling the lag disappeared and expirience became closer to proprietary note app.

Also there is an issue with releasing resources of touchHelper. For some reason the dispose() function is not called if I exit an app via navigation buttons/gestures (e.g. going to browser or launcher). So the pen continues to scribble all over the screen :)

@zinstack625
Copy link
Author

On my NoteAir 2 there is a huge lag (approx. 30s) after I lift my pen and before the actual stroke is rendered from scribble

Regarding this lag, this should be handled in scheduleRefresh(). Setting it too low may result in display refreshing too often, which in itself may be undesirable(?). 1 sec should be reasonable, but 30 secs is definitely unintended behavior. Perhaps forcing a refresh with EpdController is better than hoping it would on itself...

Also there is an issue with releasing resources of touchHelper. For some reason the dispose() function is not called if I exit an app via navigation buttons/gestures (e.g. going to browser or launcher). So the pen continues to scribble all over the screen :)

Can reproduce, dispose() is indeed not called on app minimization, and is even skipped on app close (however view is still destroyed and this sideeffect is gone). No method from PlatformView is called on this action. I guess the best option is to figure whether canvas is shown from Dart side (is there such a callback somewhere?) and pass it to Kotlin...

override fun onMethodCall(@NonNull call: MethodCall, @NonNull result: Result) {
    if (call.method == "updateStroke") {
        val params = call.arguments<Map<String, Any>?>()
        updateStroke(params)
        result.success(null)
    } else if (call.method == "disableDraw") {
        touchHelper.setRawDrawingEnabled(false)
    } else if (call.method == "enableDraw") {
        touchHelper.setRawDrawingEnabled(true)
    } else {
        result.notImplemented()
    }
}

@Fe-Ti
Copy link

Fe-Ti commented Mar 21, 2025

I guess the best option is to figure whether canvas is shown from Dart side (is there such a callback somewhere?) and pass it to Kotlin...

In such way overriding dispose() in _OnyxSdkPenAreaState may help

  @override
  void dispose() {
    channel.invokeMethod('disableDraw', creationParams).catchError((e) {});
  }

I'm not sure if it doesn't break anything in onyxsdk package interface.

@Iey4iej3
Copy link

Iey4iej3 commented Apr 5, 2025

I am unable to compile and test, but did you encounter #1459 before the fix?

@zinstack625
Copy link
Author

I am unable to compile and test, but did you encounter #1459 before the fix?

Kinda, in some circumstances, not really sure, might just be my writing. If it is there, it will probably still be there, I did not mess with the code that gets the stylus data that's used after the refresh. I however can not test anything before 800e450, stuff just straight up does not work for me until that (I have a note air 4c, which is probably too new for version used before that)

@Iey4iej3
Copy link

Iey4iej3 commented Apr 7, 2025

Kinda, in some circumstances, not really sure, might just be my writing.

The most obvious strange thing is that, the top dot in "i" is thickened into a bullet. It does not look like this in the preview, but becomes this after a refresh.

@Iey4iej3

This comment was marked as off-topic.

@Iey4iej3
Copy link

Iey4iej3 commented Apr 21, 2025

I am unable to compile and test, but did you encounter #1459 before the fix?

Kinda, in some circumstances, not really sure, might just be my writing. If it is there, it will probably still be there, I did not mess with the code that gets the stylus data that's used after the refresh. I however can not test anything before 800e450, stuff just straight up does not work for me until that (I have a note air 4c, which is probably too new for version used before that)

Actually, your patch seems to fix #1459. At least, it is much better. To clarify, it is not fixed by only upgrading onyxsdk as in 800e450. It is unclear why this patch fixes the issue. Thank you very much!

Copy link
Member

@adil192 adil192 left a comment

Choose a reason for hiding this comment

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

Really impressive, thank you! Just requesting some tiny changes

Comment on lines +28 to +34
enum class StrokeStyle(val Id: Int) {
FountainPen(0),
Pen(1),
Brush(2),
Pencil(3),
Marker(4)
}
Copy link
Member

Choose a reason for hiding this comment

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

We should define this enum in the dart package too

Comment on lines +50 to +72
int toolToOnyx(Tool currentTool) {
if (placeholder) return 5;
if (currentTool is Pencil) {
return 3;
} else if (currentTool is Highlighter) {
return 4;
} else if (currentTool is Eraser) {
return 1;
} else if (currentTool is Select) {
return 1;
} else if (currentTool is LaserPointer) {
return 1;
} else if (currentTool is Pen) {
if (currentTool.isPressureEnabled()) {
return 2;
} else {
return 1;
}
} else {
return 5;
}
}

Copy link
Member

@adil192 adil192 May 12, 2025

Choose a reason for hiding this comment

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

This can then use the enum's values instead of ints

Comment on lines +73 to +91
int getColor() {
if (currentTool is Pen) {
return (currentTool as Pen).color.toARGB32();
} else {
return Colors.black.toARGB32();
}
}
double getWidth() {
if (currentTool is Pen) {
double baseSize = (currentTool as Pen).getSize() * currentScale;
if ((currentTool as Pen).isPressureEnabled()) {
return baseSize;
} else {
return baseSize * 2;
}
} else {
return 3;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think these methods would be best named getOnyxTool, getOnyxColor, getOnyxWidth so their purpose is more clear

Comment on lines +75 to +81
bool isPressureEnabled() {
return pressureEnabled;
}
double getSize() {
return options.size;
}

Copy link
Member

Choose a reason for hiding this comment

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

These seem like redundant getters, we can just use the underlying fields themselves

Comment on lines +47 to +55
/*
Flutter ints are variable width
Personally, I think it is utterly dumb. I hope there is a way to fix
int size in flutter (why would you need 64 bits to store srgb??),
but unless or until there isn't, this stupidity should be performed
*/
val pureColor = (paramsRef?.get("strokeColor") as? Long)?.toInt()
?: paramsRef?.get("strokeColor") as? Int
?: Color.BLACK
Copy link
Member

Choose a reason for hiding this comment

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

We have the fixnum package for fixed size ints, though I think I prefer this hack over polluting the dart code

@@ -21,6 +24,9 @@ class OnyxSdkPenArea extends StatefulWidget {
/// is still writing, which will make the screen get stuck in a half-drawn
/// state.
final Duration refreshDelay;
final int strokeStyle;
Copy link
Member

Choose a reason for hiding this comment

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

This should be an enum arg too like in kotlin: e.g. strokeStyle: StrokeStyle.fountainPen

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.

4 participants