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

feat(): Android support & SyntaxHighlighterView #8

Merged
merged 3 commits into from
Mar 24, 2019

Conversation

triniwiz
Copy link
Contributor

code = "import SocketIO\n\nlet manager = SocketManager(socketURL: URL(string: \"http://localhost:8080\")!, config: [.log(true), .compress])\nlet socket = manager.defaultSocket\n\nsocket.on(clientEvent: .connect) {data, ack in\n    print(\"socket connected\")\n}\n\nsocket.on(\"currentAmount\") {data, ack in\n    guard let cur = data[0] as? Double else { return }\n    \n    socket.emitWithAck(\"canUpdate\", cur).timingOut(after: 0) {data in\n        socket.emit(\"update\", [\"amount\": cur + 2.50])\n    }\n\n    ack.with(\"Got your currentAmount\", \"dude\")\n}\n\nsocket.connect()"
<ui:SyntaxHighlighterView languageName="swift" theme="pojoaque" code="{{ code }}"/>

@shirakaba
Copy link
Owner

@triniwiz Thank you very much for this PR!

My main computer has ran out of disk space, so I'm busy installing the Android SDK and the NativeScript CLI on my older (yet more spacious) computer to test out this PR. I'm new to Android development, so may have problems getting it to run today, but will do my best.

Have you checked for any regressions on the iOS side?

@triniwiz
Copy link
Contributor Author

IOS is pretty much doing the same thing from your code 🙂

@shirakaba
Copy link
Owner

shirakaba commented Mar 10, 2019

Cool.

Having git problems...

git fetch origin pull/8/head:master

Is the correct way to check out your branch, right? It’s giving me errors.

Typing on mobile now so can’t specify exact error details.

EDIT followed these alternative instructions and have successfully checked out your branch now.

@shirakaba
Copy link
Owner

shirakaba commented Mar 10, 2019

Looks like a lot of changes to go over; may not manage to review much tonight. My summary so far:

  • Introduction of CI, wow :) I'll have to study this.
  • Introduction of tslint – works for me.
  • The demo app is certainly cleaner now. Addresses Issue Reduce (fix) the height of the example SyntaxHighlighter in the demo #5.
  • The iOS implementation looks more NativeScripty now, and indeed looks like it's set up to work as an XML component (which is a great improvement!). Addresses Issue Wrap up SyntaxHighlighter as a NativeScript component #4.
  • I'm not pleased to use a WebView-based implementation for the Android version. On the iOS version, the consumer is able to get synchronous text-change events via the HighlightDelegate and tell the underlying UITextView how to respond to those events (even to the extent that the text change may be suppressed in favour of another action, e.g. upon pressing 'tab'). Admittedly I never showed an example of using the delegate (that's Issue Show an example of invoking iOS delegate methods on the SyntaxHighlighter UITextView #6), but I do rely on this feature in my app that uses this plugin. There is also the question of the resource-usage and speed of a WebView-based implementation. Finally, by the iOS implementation being based on a native UITextView, it allows customisation of the keyboard.

Is there any way you could base the Android implementation on a TextView instead, and therefore expose text change events?

@triniwiz
Copy link
Contributor Author

triniwiz commented Mar 10, 2019

So ~2 years ago rendering styled html in the android text widget did not work like ios since you needed to handle all the unsupported styles ref1 ref2 and with the release of androidx this was added but I can't find any doc on it so atm the webview is the way to go

@shirakaba
Copy link
Owner

TextView vs. WebView

So ~2 years ago rendering styled html in the android text widget did not work like ios since you needed to handle all the unsupported styles ref1 ref2 and with the release of androidx this was added but I can't find any doc on it so atm the webview is the way to go

Okay, fair enough, if all syntax-highlighting solutions rely on HTML, and TextView doesn't support all the HTML tags needed, then we're forced to use WebView if every syntax-highlighting solution relies on HTML in some way.

I had hoped that there would be a non-HTML based competitor, but I can't immediately find one.

We could potentially handle key events by having an event handler run before setCode(). This would be useful for, e.g., converting smart quotes to code quotes; and making special actions happen on the 'return' and 'tab' (for connected keyboards) keys.

Test run

Running it on Android now.

cd demo
tns run android
chromium: [INFO:CONSOLE(8)] "Uncaught ReferenceError: hljs is not defined", source: file:///android_asset/app_sources/index.html (8)
chromium: [INFO:CONSOLE(1)] "Uncaught ReferenceError: setSource is not defined", source: file:///android_asset/app_sources/index.html (1)
chromium: [INFO:CONSOLE(1)] "Uncaught ReferenceError: changeLanguage is not defined", source: file:///android_asset/app_sources/index.html (1)
chromium: [INFO:CONSOLE(1)] "Uncaught ReferenceError: changeStyle is not defined", source: file:///android_asset/app_sources/index.html (1)

Looking into this...

@shirakaba
Copy link
Owner

shirakaba commented Mar 11, 2019

<head>
	<meta charset="utf-8">
	<link rel="stylesheet" href="index.css">
	<link id="mainstyle" rel="stylesheet" href="highlight/styles/default.css">
	<script src="highlight/highlight.pack.js"></script>
	<script>hljs.initHighlightingOnLoad();</script>
</head>

Looks like highlight.pack.js hasn't been committed.

I also noticed that the .aar file is committed; shouldn't this be left to the build process?

highlight.pack.js provided

I generated a highlight.pack.js from https://highlightjs.org/download/, selecting all languages to be included. The resulting file was 686 KB.

I provided this, which stopped the hljs is not defined error.

However, the last three errors still remained; did you define these functions yourself, writing them into highlight.pack.js or something?

If so, it would be preferable to inject them as pre-page-load JS snippets via syntax-highlighter.android.ts rather than polluting highlight.js's distributed pack.

@triniwiz
Copy link
Contributor Author

Some .js files didn’t make it

@shirakaba
Copy link
Owner

Awesome, that's working now.

Improvements to make:

Android

  • fit the <body> to the screen width
  • provide a 'loading...' prompt somehow because the WebView takes several seconds to appear.
  • implement key events
  • Ensure somehow that the 'coding' style keyboard appears during editing mode, with autocorrect and capitalisation turned off.

iOS

  • integrate the delegate, then implement key events

I'll see what I can do on all of these.

@shirakaba
Copy link
Owner

  • I tried adjusting the <body> to fit the screen width. I've included a viewport meta tag and set every element to width: 100%, but they're not filling the width; not sure why.
  • Out of the available keyboard styles, I think text comes closest. However, in HTML, only the <input> element gives any control over the associated keyboard type – and that's a single-line element.
  • I found that the syntax highlighting is not updated upon changing the text.

Unless we can prevent auto-correct and auto-capitalisation on the keyboard, and implement live-updating of syntax highlighting, this plugin is unusable for writing code. It can work for just displaying code, but even then, we'd first need to find a way to fit it to the screen properly.

Do you have any ideas?

@shirakaba
Copy link
Owner

Solving the syntax highlighting not updating on text change

This may just require a bit of rethinking the javascript we're using.

Alternative projects to possibly solve the keyboard issue

I looked into the various options for Android.

I noticed that my iOS plugin uses highlight.js for its logic, converting the HTML to native NSAttributedStrings – it's just that iOS's UITextView supports more of HTML than Android's TextView does!

Directly solving the keyboard issue

There may be a way to do this.

This article details how to manage the keyboard on WebView by extending the WebView class. This is probably our best option from here. ✅

Solving the width issue

I'm still unsure why highlightr doesn't fill the phone screen. Maybe it has a minimum width..?

@shirakaba
Copy link
Owner

Another thing I noticed: if you keep adding new lines to the text box in the current implementation's WebView, the page doesn't scroll down to keep the current line in view.

It seems like the writing experience in general is bad. I wonder whether we should accept that this plugin will mainly be for displaying text rather than editing it, given that Android has no proper text editors. But 920 Text Editor seems to have done a good job...

@shirakaba shirakaba merged commit fce6db4 into shirakaba:master Mar 24, 2019
@shirakaba
Copy link
Owner

shirakaba commented Mar 24, 2019

@triniwiz I've had a moment to test the iOS version. No regressions as far as I can see.

Given that this PR improves the code quality of the iOS side of the codebase, introduces an XML component, and provides foundational Android support, I'm very happy to merge it. I feel that the remaining Android issues that I've commented on can be deferred to separate PRs for anyone brave enough to solve them.

Thank you very much for the high-quality PR! It serves as great guidance for me writing future plugins.

@triniwiz
Copy link
Contributor Author

Glad you liked it 🙂

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.

2 participants