add code floading - #873
Conversation
|
adding custom cursor by #853 |
|
I haven't looked into the code in detail, but this is presumably entirely vibe-coded. The amount of comments and setting the author of the generated classes to So the review process should probably be more skeptical. |
No, like you, we get a lot of help from AI. I downloaded the code from GitHub and then spent a week fixing it. AI only played the role of diagnosing and solving the problem, and the feature works well. We wait for the developer, not other people. |
|
This aged well. My message didn’t imply anything personal, and I never said this PR shouldn’t be merged. I simply wrote this to ensure that this well-written library, used by hundreds of codebases, remains well-written and thoroughly thought-out. Because in my experience, AI-generated PRs lack this depth of understanding. After my message, in another repository, the same person made insulting and racist comments about me and another person who had reacted to the message above. Part of the message:
(I am not Indian...) @Rosemoe Apart from the technical side, I think it's now fair to ask, whether merging the PR of people who behave this way is consistent with the moral values of this project. People with such malicious intentions should not end up in a public repository. |
dingyi222666
left a comment
There was a problem hiding this comment.
Review: Request Changes
This PR bundles three unrelated features (code folding, cursor styles + blinking animations, ghost-text inlay hints), a whole-file reformat of EditorSearcher.java and I18nConfig.java, and an unrelated dependency removal in app/build.gradle.kts. That alone should be split into 3-4 PRs; as-is the diff is effectively unreviewable (the real feature logic is buried in ~1100 lines of formatting noise).
On the folding implementation itself, the fundamental problem is that folding is not modeled where it belongs — in the layout. AbstractLayout already owns the row↔line mapping and all visual coordinates. Instead, this PR:
- Introduces a
FoldingManagerthat holds aCodeEditorreference and exposes folding-specific queries to the whole widget package. - Threads
editor.isFoldingEnabled()/editor.getFoldingManager()/editor.isLineHiddenByFolding()through every method of bothLineBreakLayoutandWordwrapLayout(and even intoRowIterators). The layout now asks the editor "what should I render?" instead of providing the mapping. That is an inversion of the first abstraction principle. - Maintains two independent copies of the visible-row mapping (
FoldingManager.visibleLines/lineToVisibleRowandWordwrapLayout.visibleRowToTableRow/tableRowToVisibleRow) with separate invalidation versions — two sources of truth that will drift. - Adds
Styles.blocksByStart, a redundant sorted duplicate ofblocksthat is only populated infinishBuilding().Styles.adjustOnInsert/adjustOnDeleteshiftblocksbut never updateblocksByStart, so every foldableendLinegoes stale the moment the user types. That is not a design opinion — it is a user-visible correctness bug (see comments).
The correct shape: folding belongs inside the layout layer as a row-space transformation. Cursor, selection, touch, renderer and minimap already work in row space; folding should not require branching on editor.isFoldingEnabled() in every method. The language layer can suggest foldable ranges; the layout owns the mapping.
Line-level findings are inline (~25 issues). The two worst are:
Styles.blocksByStartstaleness — folding targets wrong lines after any text edit.- ~200-line hand-rolled brace/comment parser in
drawFoldingPlaceholder— a language-agnostic editor cannot guess syntax this way; it will show wrong output on strings/templates/regex and allocates Strings on every frame.
Additionally, unrelated features have their own problems (60fps infinite postInvalidate loop in CursorBlink for non-BLINK modes; the foldingEnabled = true default silently changes the gutter and tap behavior for every existing user).
| /** | ||
| * Internal, automatically generated | ||
| */ | ||
| public List<CodeBlock> blocksByStart; |
There was a problem hiding this comment.
This field should not exist. It is a second, separately-sorted copy of blocks, kept only so the folding manager can iterate by start line. Two sources of truth for the same data. And it is not kept in sync (see below), which makes it actively harmful.
| if (sort) { | ||
| Collections.sort(blocks, CodeBlock.COMPARATOR_END); | ||
| } | ||
| blocksByStart = new ArrayList<>(blocks); |
There was a problem hiding this comment.
blocksByStart is only created in finishBuilding(). adjustOnInsert/adjustOnDelete (lines 139-148) shift blocks but never touch blocksByStart. So the moment the user types anything that changes line numbers, every foldable endLine in blocksByStart is stale, and FoldingManager will hide/show the wrong rows until a full re-analyze. This is a correctness bug caused by the redundant field.
| */ | ||
| private long mappingVersion = 0; | ||
|
|
||
| public FoldingManager(@NonNull CodeEditor editor) { |
There was a problem hiding this comment.
FoldingManager holds a CodeEditor (and is held by it) — a circular peer coupling. This class mixes fold state, visible-row mapping, and editor queries (getLineCount(), getProps()). The visible-line mapping maintained here is exactly what AbstractLayout should own. Inverting this (layout owns the mapping, language only supplies foldable ranges) would delete ~40 call sites of editor.isFoldingEnabled() / editor.getFoldingManager() scattered through the layouts.
| * | ||
| * @return true 表示“隐藏区间”发生变化,需要刷新布局/滚动范围 | ||
| */ | ||
| public boolean onStylesUpdated(@Nullable Styles styles) { |
There was a problem hiding this comment.
Foldables are re-derived from Styles.blocksByStart on every style update, and the return value (a hash of hidden ranges) decides in CodeEditor.setStyles/updateStyles whether to rebuild the layout. The async analyzer's style snapshot now drives layout structure. Folding state should be owned by the layout and driven by text/layout events, not re-derived from a stale style list.
There was a problem hiding this comment.
Yes, I checked the code. It actually creates a copy and may cause a bug. I'm fixing this problem on my to-do list.
| continue; | ||
| } | ||
| final int oldEnd = foldableEndsByStartLine.get(startLine, -1); | ||
| if (endLine > oldEnd) { |
There was a problem hiding this comment.
Nested blocks are destroyed here: for each startLine only the maximum endLine is kept, so inner block extents are discarded. You can never independently fold an inner block — only the outermost extent. Nested constructs (method > for > if) are the normal case in real code and this model cannot represent them.
| * to trigger a layout refresh. | ||
| */ | ||
| @InvalidateRequired | ||
| public boolean foldingEnabled = true; |
There was a problem hiding this comment.
foldingEnabled = true by default is a silent, breaking default for every existing user: the gutter gets wider, icons appear, tap behavior changes. New opt-in features should default off. Also the Javadoc says to call setFoldingEnabled() when mutating this public field — a second source of truth for the same flag.
| @Override | ||
| public void run() { | ||
| if (valid && period > 0) { | ||
| boolean smooth = editor.getCursorBlinkingType() != CursorBlinkingType.BLINK; |
There was a problem hiding this comment.
For any non-BLINK type this posts a 16ms delayed runnable forever — a permanent ~60fps postInvalidate loop on a static editor, even when the cursor is not visible. There is no throttling or stop condition when the editor is idle. This will drain battery in any app that enables these cursor styles.
| * every following line is anchored to the start (column 0) of the | ||
| * corresponding following line. | ||
| */ | ||
| fun split(line: Int, column: Int, text: String): List<GhostTextInlayHint> { |
There was a problem hiding this comment.
Out-of-scope feature inside the folding PR. Also split() uses text.split('\n') so a trailing newline creates an empty last hint, and subsequent lines anchored at column 0 of line + index are silently wrong if the anchor line exceeds the document.
| typefaceText = typeface | ||
| props.stickyScroll = true | ||
| setLineSpacing(2f, 1.1f) | ||
| cursorType = CursorType.UNDERLINE |
There was a problem hiding this comment.
Demo app changes forcing CursorType.UNDERLINE + PHASE on the shared editor, plus ghost-text demos with hard-coded line/column (11,40 / 100,0) that are wrong for most sample files, plus a mis-indented demoInlayHintProvider block below. These belong in a separate demo PR, not forced on the default editor.
| implementation(projects.languageMonarch) | ||
| implementation(projects.editorLsp) | ||
| implementation(projects.languageTreesitter) | ||
| implementation(projects.onigurumaNative) |
There was a problem hiding this comment.
Removing the onigurumaNative dependency is unrelated to this PR and breaks the demo's MonarchLanguage regex support.
There was a problem hiding this comment.
First, it can't be used. I didn't see a place for it to be used. It only compiles native code. Second, the problem is with my IDE. It can't compile native code. It's nothing special. You can add a line yourself.
|
@dingyi222666 Okay, I'll fix it in the evening. |
|
Please use android studio.
…---Original---
From: ***@***.***>
Date: Tue, Aug 11, 2026 20:06 PM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [Rosemoe/sora-editor] add code floading (PR #873)
@Ghostreal30 commented on this pull request.
In editor/src/main/java/io/github/rosemoe/sora/lang/styling/inlayHint/GhostTextInlayHint.kt:
> + val text: String, + displaySide: CharacterSide = CharacterSide.LEFT +) : InlayHint(line, column, TYPE_NAME, displaySide) { + + companion object { + const val TYPE_NAME = "ghost-text" + + /** + * Split a (potentially) multi-line ghost text into a list of + * [GhostTextInlayHint], one per line. + * + * The first line is anchored at the given [line] and [column], while + * every following line is anchored to the start (column 0) of the + * corresponding following line. + */ + fun split(line: Int, column: Int, text: String): List<GhostTextInlayHint> {
oh ok tnks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I don't have a laptop and it exploded, but don't worry, I'll fix it in the next few days. |
|
Also, please make sure that the JavaDocs are written in English. |
style: reformat EditorSearcher and I18nConfig back to 4-space indent 8a70904 build(sample): restore onigurumaNative dependency fix(sample): drop forced cursor style, fix ghost-text demo block fix(inlayHint): drop trailing empty ghost hint, clamp to document bounds perf(cursor): stop the smooth blink from polling at 60fps when idle aef7f1b fix(widget): default foldingEnabled to false refactor(folding): decouple FoldingManager from CodeEditor, keep nested fold regions fix(styling): remove Styles#blocksByStart, the stale duplicate block list chore: import project tree (baseline for review-fix commits)
|
@dingyi222666 see new Change |
|
Please ensure that you have addressed all review comments and architectural design issues. Otherwise, this code cannot be merged. |
|
I did everything, but then I don't have much time. I have to participate in big projects. If you would like, you can participate yourself. |
|
@dingyi222666 Check the codes, contact me if there is a problem. |
|
@Rosemoe @dingyi222666 Is there anyone who can answer? |
Folding code feature! Yes, this is what was hard to build years ago, but now it's within reach.