-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Batch editing for Search & Replace #59
base: master
Are you sure you want to change the base?
Conversation
# Batch Linking - Select one or more blocks (full blue highlight…not edit mode) - Press `Meta+shift+l` - Type a word or phrase to batch-convert to [[links]] - Press `Enter` Note: Batch Linking purposefully matches on whole-word boundaries only. This avoides accidentally linking words within other words, so batch-linking `ears` will turn `ears` into `[[ears]]`, but will leave `earshot` alone. This is in contrast to Roam's "Link All" feature, which will happily create a link to your page that looks like this: `[[ears]]hot`. # End Tags - Select one or more blocks (full blue highlight…not edit mode) - Press `Ctrl+shift+t` - Type a word to use as a tag - Press `Enter` The word you typed will be appended to every block, so… • One block • Another block …becomes… • One block #tagged • Another block #tagged # Known bugs - ✅ `tag` appends ` #tag`, but ❌ `urgent tag` appends ` #urgent tag`. - ❌ A number of roam-toolkit behaviors seem to break Undo, and these functions are no exception - ❌ While thse functions preserve the highlighted blocks, making it easy to toggle tags on-and-off, but the highlight is overly-permanent. Workaround: reload the page.
… batch operations
- tag → #tag - compund tag → #[[compound tag]] Note: batch tag removal works with this change out of the box
Thanks! will review soon-ish |
First comment from just trying to use it - it should work for current active block too (i.e. if it's in edit mode) |
Also the highlighting bug is confusing. If we can't fix it I'd prefer to loose the highlighting rather then to have it |
I'm surprised that this breaks undo, other toolkit functions don't seem to. |
btw can you elaborate on use-case for add/remove tag at the end? |
I think looking at #63 would be useful for figuring out the highlighting stuff |
const regex = new RegExp(`\\[\\[${text}]]|(\\b${text}\\b)`, 'g') | ||
return originalString.replace(regex, function (m, group1) { | ||
if (!group1) return m | ||
else return `[[${m}]]` | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be made simpler by using regex like [^\\[]\\b${text}\\b[^\\]]
and then just replacing all matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this…
const regex = new RegExp(`[^\\[]\\b${text}\\b[^\\]]`, 'g')
return originalString.replace(regex, `[[${text}]]`)
…and it doesn't work:
Create a Bracket Link.
becomes
Create a[[Bracket Link]]
¯_(ツ)_/¯
if (!text || text === '') return | ||
|
||
withHighlightedBlocks(originalString => { | ||
if (text.includes(' ')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a bunch of other things just #
does not work with. I think it'd be simpler to just always add brackets
return | ||
|
||
withHighlightedBlocks(originalString => { | ||
const regex = new RegExp(`(.*) (#.*)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not seem like it would handle newlines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also if you don't need to use template string/etc the /(.*) (#.*)/
syntax is a bit nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just doing something like "find last #
" and take a substring before that as a result may be simpler here vs using regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good potential catch. I tested it out and it actually seems to work fine with newlines, assuming you mean the sort that allow you to add whitespace within a single block (Shift-return on my Mac).
Good point re: template string. I've replaced it with an inlined regex literal. :)
const withHighlightedBlocks = (mod: { (orig: string): string }) => { | ||
const highlighted = getHighlightedBlocks() | ||
|
||
const contentBlocks = Array.from(highlighted.contentBlocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoul getHighlightedBlocks return array straight away?
if (!textarea) { | ||
console.log("🚨 NO TEXTAREA returned from ", element) | ||
return | ||
} | ||
|
||
const newText = mod(textarea.value) | ||
|
||
Roam.save(new RoamNode(newText)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the apply
function that does basically this
Keyboard.pressEsc() | ||
Keyboard.pressEsc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I'm not sure if this should be responsibility of this function
@@ -77,6 +84,20 @@ export const Roam = { | |||
this.applyToCurrent(node => node.withCursorAtTheEnd()) | |||
}, | |||
|
|||
async replace(element: HTMLElement, mod: { (orig: string): string }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is basically apply but for HtmlElement instead of current block, I feel like they should follow similar interface (i.e. in terms of RoamNode)
// resulting block before Roam has had a chance to | ||
// replace the <span> with a <textarea>. Without this, | ||
// Batch operations often skip some blocks. | ||
await delay(100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftClick
has additional delay parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also is 100 an absolute minimum we can live with?
@@ -33,6 +33,18 @@ export function getFirstTopLevelBlock() { | |||
return firstChild.querySelector('.roam-block, textarea') as HTMLElement | |||
} | |||
|
|||
export function getHighlightedBlocks(): { parentBlocks: NodeListOf<HTMLElement>, contentBlocks: NodeListOf<HTMLElement> } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably be 2 separate functions
This PR constitutes a method for batch editing of Roam blocks and a handful of implemented scenarios
Features Demonstrated
Watch a quick demo of the first 2 bullet points here, and a demo pertinent to #35 here. 🤓
Implementation Notes
Making new variants of this workflow is super easy! Just pass a text manipulation function into
withHighlightedBlocks
.To make this work, I've added 3 general-purpose functions:
getHighlightedBlocks()
which returns parallel arrays of the currently-highlighted blocks and the content blocks nested thereinforEachAsync()
used to simulate async actions on each blockRoam.replace()
which takes a string -> string function as an argument and uses it to flexibly update a node's contentsKnown Issues
This PR is a minimum-viable-product implementation of search-and-replace. The ideal UI for this sort of thing would allow each replacement to be reviewed one step at a time, but that would require far more design work.
When implementing the add/remove tag features, I found myself wanting the highlighted nodes to "remain" highlighted after the change. As a result,
withHighlightedBlocks
does a naive "re-highlighting" after all of the blocks have been processed. Unfortunately, the highlight state appears to be maintained separately within Roam itself, resulting in blocks that remain highlighted even after hittingesc
.esc
to re-syncs the highlight stateMy hope is that someone smarter than me can find a better way to reestablish the highlights…perhaps by simulating keystrokes? If not, it may be worth deleting batch-editing.ts:106-113