Conversation
m-abs
commented
Apr 8, 2026
- feat(NotaComicBooks): handle nota comic books.
- feat(NotaComic): Full page rendering
- fix(android): location change were delayed, which broke animations
- update comic css
|
Missing iOS part
|
There was a problem hiding this comment.
Pull request overview
Adds a new “Nota comic book” rendering path by moving comic handling into the injected Readium helper bundle, introducing full-page overlay rendering (with optional B/W mode), and wiring segment-duration-based panning to synced audio/navigation. Also refactors Android preference plumbing to a richer, typed FlutterEpubPreferences model and updates helper-scripts tooling/demo setup.
Changes:
- Replace the legacy
comics.*injected assets with a newNotaComicBookPageimplementation bundled intoflutterReadiumTools.*. - Add/propagate a
blackAndWhiteComicModepreference and update Android preference parsing/storage toFlutterEpubPreferences. - Improve Android navigation/sync behavior (segment duration propagation, script/style injection filtering, and sync-audio locator handling).
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| flutter_readium/ios/flutter_readium/Sources/flutter_readium/ReadiumReaderView.swift | Updates user-script injection and ToC script; currently leaves broken refs to removed comics assets. |
| flutter_readium/example/lib/state/text_settings_bloc.dart | Adds blackAndWhiteComicMode to state and forwards it into EPUB preferences. |
| flutter_readium/assets/helpers/flutterReadiumTools.js | Updates the bundled injected helper JS (now includes Nota comic logic). |
| flutter_readium/assets/helpers/flutterReadiumTools.css | Updates the bundled injected helper CSS for Nota comic overlay + existing styles. |
| flutter_readium/assets/helpers/comics.js | Removes legacy comics bundle. |
| flutter_readium/assets/helpers/comics.css | Removes legacy comics styles. |
| flutter_readium/assets/_helper_scripts/webpack.config.ts | Stops building the legacy comics entrypoint. |
| flutter_readium/assets/_helper_scripts/src/types.ts | Renames/extends types and adds figureQuerySelector + window.readium typing. |
| flutter_readium/assets/_helper_scripts/src/NotaComicBookPage.ts | New Nota comic overlay renderer and Readium scroll interception. |
| flutter_readium/assets/_helper_scripts/src/NotaComicBookPage.scss | New overlay CSS + B/W filter + (intended) scroll lock + area hiding. |
| flutter_readium/assets/_helper_scripts/src/FlutterReadiumTools.ts | Integrates Nota comic initialization and adds segment-duration plumbing + global helpers. |
| flutter_readium/assets/_helper_scripts/src/demo/DemoComicPanning.ts | Updates demo to use flutterReadiumTools.* instead of legacy comics.*. |
| flutter_readium/assets/_helper_scripts/src/ComicBookPage.ts | Removes legacy comic renderer implementation. |
| flutter_readium/assets/_helper_scripts/src/ComicBookPage.scss | Removes legacy comic renderer styles. |
| flutter_readium/assets/_helper_scripts/src/ComicBookCalc.ts | Refactors keyframe generation + adds full-page frame calc + timing tweaks. |
| flutter_readium/assets/_helper_scripts/public/books/xkcd/index.html | Updates demo markup to figure + areas format expected by Nota comic logic. |
| flutter_readium/assets/_helper_scripts/public/books/comic-panning/media-overlay.json | Removes old demo book overlay. |
| flutter_readium/assets/_helper_scripts/public/books/comic-panning/index.html | Removes old demo book HTML. |
| flutter_readium/assets/_helper_scripts/package.json | Adds dev-server dep and adjusts scripts. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/Utils.kt | Adds helpers to convert dynamic maps into kotlinx JsonElement for prefs decoding. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/ReadiumReaderWidget.kt | Switches preferences parsing to FlutterEpubPreferences with Map<String, Any>. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/ReadiumReader.kt | Filters script/style injection to HTML-like resources; adds segmentDuration plumbing. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/ReadiumExtensions.kt | Removes legacy comics injection; exposes readiumColorFromCSS for new prefs model. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/navigators/SyncAudiobookNavigator.kt | Uses a flow-driven locator sync (with segment-duration propagation) and removes last-item tracking. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/navigators/EpubNavigator.kt | Stores FlutterEpubPreferences, updates preferences + toggles B/W comic mode via JS, and pushes segment duration into JS. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/models/FlutterMediaOverlayItem.kt | Adds computed segment duration. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/models/FlutterMediaOverlay.kt | Uses mediaType.isHtml instead of explicit MediaType checks. |
| flutter_readium/android/src/main/kotlin/dk/nota/flutter_readium/FlutterEpubPreferences.kt | New serializable preferences model + conversion to Readium EpubPreferences. |
| flutter_readium_platform_interface/lib/src/reader/reader_tts_preferences.dart | Minor typing improvement for voices map. |
| flutter_readium_platform_interface/lib/src/reader/reader_epub_preferences.dart | Major expansion of EPUB prefs model + JSON parsing/serialization + new comic mode flag. |
| .vscode/tasks.json | Adds helper-scripts build task. |
| .vscode/settings.json | Marks generated/bundled JS/CSS as excluded + readonly in VS Code. |
| .vscode/launch.json | Adds preLaunch helper build and a demo launcher. |
| .vscode/extensions.json | Recommends ESLint/Prettier extensions. |
Comments suppressed due to low confidence (1)
flutter_readium/ios/flutter_readium/Sources/flutter_readium/ReadiumReaderView.swift:434
initUserScriptsstill referencescomicJsKey/comicCssKeyin the script arrays, but their declarations were removed. This will not compile, and even if reintroduced it would crash at runtime ifcomics.js/.cssassets are no longer shipped. Remove the comics entries fromjsScripts/addCssScripts(or re-add the lookup keys and ensure the assets exist) so only the bundledflutterReadiumTools.*assets are injected.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| #onResize = (): void => this.scrollToId(this.#lastElementId); |
There was a problem hiding this comment.
#onResize calls scrollToId(this.#lastElementId) even when #lastElementId is still null (e.g. before any navigation), which will trigger the "scrollToId called with empty id" warning on every resize. Guard against null (e.g., only call when #lastElementId is truthy) to avoid noisy logs and unnecessary work.
| #onResize = (): void => this.scrollToId(this.#lastElementId); | |
| #onResize = (): void => { | |
| if (this.#lastElementId) { | |
| this.scrollToId(this.#lastElementId); | |
| } | |
| }; |
| class TextSettingsState { | ||
| TextSettingsState({ | ||
| required this.verticalScroll, | ||
| required this.fontSize, | ||
| required this.theme, | ||
| required this.highlight, | ||
| this.pageMargins, | ||
| this.blackAndWhiteComicMode = false, | ||
| }); | ||
|
|
||
| bool verticalScroll; | ||
| int fontSize; | ||
| TextSettingsTheme theme; | ||
| TextSettingsTheme highlight; | ||
| double? pageMargins; | ||
| bool blackAndWhiteComicMode; | ||
|
|
There was a problem hiding this comment.
blackAndWhiteComicMode was added to the state, but TextSettingsState.copyWith() doesn’t allow updating it. Any future toggle event will be forced to recreate state manually (or will silently fail to update this field). Add a blackAndWhiteComicMode parameter to copyWith and pass it into the new TextSettingsState instance.
| final verticalScroll = jsonObject.optNullableBoolean('verticalScroll', remove: true); | ||
| final spread = jsonObject.opt('spread', remove: true); // Replace with actual parsing if needed | ||
| final textAlign = jsonObject.opt('textAlign', remove: true); // Replace with actual parsing if needed | ||
| final textColor = jsonObject.optNullableString('textColor', remove: true); | ||
| final textNormalization = jsonObject.optNullableBoolean('textNormalization', remove: true); |
There was a problem hiding this comment.
spread and textAlign are read with opt() and passed through without parsing. textAlign is declared as TextAlign?, so if the JSON provides a string (as is typical), this will throw a runtime type error. Use optNullableString() and parse via TextAlign.fromJson(...) (and similarly for any other enum-like fields) to make JSON parsing robust.
| final themeStr = jsonObject.optNullableString('theme', remove: true); | ||
| final theme = ThemeType.fromJson(themeStr); | ||
| final typeScale = jsonObject.optDouble('typeScale', remove: true); | ||
| final verticalText = jsonObject.optNullableBoolean('verticalText', remove: true); | ||
| final wordSpacing = jsonObject.optDouble('wordSpacing', remove: true); | ||
| final blackAndWhiteComicMode = jsonObject.optBoolean('blackAndWhiteComicMode', remove: true); |
There was a problem hiding this comment.
typeScale/wordSpacing are read using optDouble() without checking key presence. When missing, optDouble defaults to double.nan, which then gets serialized back out and can poison downstream preference merging. Use optNullableDouble() (or provide a sane fallback) so absent values remain null.
| @override | ||
| Map<String, dynamic> toJson() => {} | ||
| ..putOpt('backgroundColor', backgroundColor?.toCSS()) | ||
| ..putOpt('columnCount', columnCount?.toJson()) | ||
| ..putOpt('fontFamily', fontFamily) | ||
| ..putOpt('fontSize', fontSize) | ||
| ..putOpt('fontWeight', fontWeight) | ||
| ..putOpt('hyphens', hyphens) |
There was a problem hiding this comment.
toJson() now emits non-string values (ints/doubles/bools/enums), but the iOS plugin side currently casts the method-channel arguments to [String: String] (see ReadiumReaderView.swift), which will crash at runtime. Either keep encoding values as strings for compatibility, or update the iOS handler and its EPUBPreferences.init(fromMap:) to accept Any values like Android now does.
| declare const readium: Readium | undefined; | ||
|
|
||
| export class FlutterReadiumTools { | ||
| class FlutterReadiumTools { | ||
| get #isScrollModeEnabled(): boolean { | ||
| return readium.isReflowable === true && getComputedStyle(document.documentElement).getPropertyValue('--USER__view')?.trim() === 'readium-scroll-on"'; | ||
| return readium?.isReflowable === true && getComputedStyle(document.documentElement).getPropertyValue('--USER__view')?.trim() === 'readium-scroll-on"'; | ||
| } |
There was a problem hiding this comment.
readium is now declared as Readium | undefined, but the rest of this file still assumes it exists in several code paths (e.g. methods that call into readium.* without guarding). If this bundle is loaded in contexts without the injected Readium API (like the helper-scripts demo pages), calling those methods will throw at runtime. Either make readium non-optional again (if it’s guaranteed), or consistently guard accesses and return sensible fallbacks when it’s missing.
| import org.readium.r2.shared.ExperimentalReadiumApi | ||
| import org.readium.r2.shared.publication.Locator | ||
| import org.readium.r2.shared.publication.Publication | ||
| import org.readium.r2.shared.publication.html.cssSelector |
There was a problem hiding this comment.
The import org.readium.r2.shared.publication.html.cssSelector appears unused in this file (the code references locator.locations.cssSelector directly). Removing it will avoid an unused import warning and keep dependencies tidy.
| import org.readium.r2.shared.publication.html.cssSelector |
There was a problem hiding this comment.
This is incorrect org.readium.r2.shared.publication.html.cssSelector is a helper needed for locator.locations.cssSelector
| const figureId = figureElement.id; | ||
| const comicImgId = comicImg.id || figureId; | ||
| if (comicImgId == figureId) { | ||
| delete figureElement.id; |
There was a problem hiding this comment.
delete figureElement.id doesn’t clear the element’s id attribute (it only attempts to delete an own JS property, and id is an accessor on the prototype). In the common case where comicImg.id is empty, this leaves both the <figure> and <img> sharing the same id, which can break getElementById lookups. Use figureElement.removeAttribute('id') or figureElement.id = '' instead.
| delete figureElement.id; | |
| figureElement.removeAttribute('id'); |
…Locator from matching them
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 51 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
flutter_readium/ios/flutter_readium/Sources/flutter_readium/ReadiumReaderView.swift:433
initUserScripts()still referencescomicJsKey/comicCssKeyin thejsScripts/addCssScriptsarrays, but the lookupKey definitions for those variables were removed in this PR. This will fail to compile, and it also looks inconsistent with Android wherecomics.js/comics.cssinjection was removed. Either reintroduce the variables or (more likely) drop the comics assets entirely here and only injectflutterReadiumTools.js/flutterReadiumTools.css.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import CopyPlugin from 'copy-webpack-plugin'; | ||
| import HtmlWebpackPlugin from 'html-webpack-plugin'; | ||
| import webpack from 'webpack'; | ||
| import { getEnvVariables, outputPath, resolveApp } from './webpack.tools'; |
| import TerserPlugin from 'terser-webpack-plugin'; | ||
| import webpack from 'webpack'; |
| import MiniCssExtractPlugin from 'mini-css-extract-plugin'; | ||
| import * as webpack from 'webpack'; | ||
| import { merge } from 'webpack-merge'; |
| /// Font size for text content. | ||
| final int? fontSize; | ||
|
|
| final scroll = jsonObject.optNullableBoolean('scroll', remove: true); | ||
| final spread = jsonObject.opt('spread', remove: true); | ||
| final textAlign = jsonObject.optEnumFromString('textAlign', TextAlign.values, remove: true); |
| this.#container.classList.remove(activeComicPageContainerClass); | ||
| for (let child of this.#container.childNodes) { | ||
| this.#container.removeChild(child); | ||
| } |
| @override | ||
| Map<String, dynamic> toJson() => {} | ||
| ..putOpt('backgroundColor', backgroundColor?.toCSS()) | ||
| ..putOpt('columnCount', columnCount?.toJson()) | ||
| ..putOpt('fontFamily', fontFamily) | ||
| ..putOpt('fontSize', fontSize) | ||
| ..putOpt('fontWeight', fontWeight) | ||
| ..putOpt('hyphens', hyphens) | ||
| ..putOpt('imageFilter', imageFilter?.toJson()) | ||
| ..putOpt('language', language) | ||
| ..putOpt('letterSpacing', letterSpacing) | ||
| ..putOpt('ligatures', ligatures) | ||
| ..putOpt('lineHeight', lineHeight) | ||
| ..putOpt('pageMargins', pageMargins) | ||
| ..putOpt('paragraphIndent', paragraphIndent) | ||
| ..putOpt('paragraphSpacing', paragraphSpacing) | ||
| ..putOpt('publisherStyles', publisherStyles) | ||
| ..putOpt('readingProgression', readingProgression?.toJson()) | ||
| ..putOpt('scroll', scroll) | ||
| ..putOpt('spread', spread) | ||
| ..putOpt('textAlign', textAlign?.name) | ||
| ..putOpt('textColor', textColor?.toCSS()) | ||
| ..putOpt('textNormalization', textNormalization) | ||
| ..putOpt('theme', theme?.toJson()) | ||
| ..putOpt('typeScale', typeScale) | ||
| ..putOpt('verticalText', verticalText) | ||
| ..putOpt('wordSpacing', wordSpacing) | ||
| ..put('blackAndWhiteComicMode', blackAndWhiteComicMode) | ||
| ..put('disableSynchronization', disableSynchronization); |
| 'preceding::*[@epub:type="pagebreak" or @type="pagebreak" or @role="doc-pagebreak" or contains(@class,"pagebreak")][1]', | ||
| element, | ||
| (prefix: string) => { | ||
| (prefix: string | null) => { |
| /** | ||
| * Prevent scrolling of the body when a comic book page is active. | ||
| */ | ||
| body:has(div.nota-comicbook-page-container.active) { |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ddfreiling
left a comment
There was a problem hiding this comment.
Looks good so far, minor comment on the casing here.
| epubNavigator?.updatePreferences(preferences.toEpubPreferences()) | ||
|
|
||
| if (preferences.blackAndWhiteComicMode != oldBlackAndWhiteComicMode) { | ||
| epubNavigator?.evaluateJavascript("window.SetBlackAndWhiteMode(${preferences.blackAndWhiteComicMode})") |
There was a problem hiding this comment.
I think this should follow camelCase like the rest of our JS interface.