bamaer commented on PR #8423: URL: https://github.com/apache/hop/pull/8423#issuecomment-5702697608
Reviewed this after it landed, so this is follow-up rather than pre-merge feedback. Good catch on the root cause. `setTextSuppressModify` guards with a plain boolean in a try/finally — fine for the synchronous RCP SourceViewer, but the RAP Monaco widget echoes `contentChanged` back from the browser long after `finally` resets the flag (`ContentEditorFacadeImpl` 326-333 vs. handler at 249-256). Hence every restore flipped dirty. `cleanText` is the right fix; `setText` updates `cachedContent` synchronously, so `markClean()` captures the correct baseline before the echo lands. Builds clean (`-pl ui -am install` + `spotless:check`); the 5 database-perspective test classes pass. Note: the `restoringSqlTabs` guard in `onEditorModified` doesn't contribute in the web case — the echo arrives after `restore()` resets it. `cleanText` does the work. **Two things worth a follow-up:** **1. In-session tab moves lose the dirty flag.** Every `DatabaseWorkbench` calls `restore()` in its constructor (`DatabaseWorkbench:286`), and `openInBottomDock` / `restoreIntoRemaining` route same-session moves through `handOff` → persist → restore. `restoredTabIsDirty` can't tell a cold start from a live move, so an untitled tab edited thirty seconds ago comes back non-bold and closes silently after a move to the dock. Could we thread a `coldStart` flag from `restore()` into `restoreSqlTab` and honour `snapshot.dirty` on a live handoff? **2. `markRestoredSqlTabsClean()` is redundant.** `applyBuffer`/`loadFromVfs` set the baseline and `restoreSqlTab` already calls `markClean()` for non-dirty tabs — this is a third pass. Its only unique coverage is `openSqlTab` returning an existing tab on a duplicate filename, which is also the risk: it's the one place that can clear a `changed` flag it didn't set, and it duplicates the untitled-is-always-clean policy. I'd drop it. **Design question:** `isCloseable()` returns `true` immediately when `!changed`, and `disposeTab` drops the snapshot. So closing a restored untitled tab now discards the buffer with no prompt and no undo. Intended for shutdown per #8366, but it also covers single-tab close, where the prompt was useful. Should `isCloseable()` still prompt for an untitled tab with content? **Minor:** - `onEditorModified` doesn't dispose-check `editor` the way `markClean` does; RCP fires it synchronously, so a dispose-time event hits `editor.getText()` unguarded. - `restoredTabIsDirty(null)` is unreachable — `restoreSqlTab` dereferences `snapshot.connection` two lines earlier. That assertion tests dead code. - `applyBuffer` sets `cleanText` before checking `markDirty`, so a tab created dirty gets its dirty text as baseline. Harmless today, but reads as a bug. - `markClean()` could be package-private; `setInitialText` has no callers anywhere. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
