mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-25 04:54:07 +00:00
Enhance WhitespaceUtil and Reconciler to handle duplicate names and improve diffing logic
- Updated WhitespaceUtil to return distinct values for totalDiff and whitespaceDiff when differences exceed 100 lines, based on content changes. - Introduced findDuplicateNames function in Reconciler to identify and skip instances with duplicate names, ensuring reliable syncing. - Enhanced diffing logic to account for ambiguous paths and log skipped duplicates during the sync process. - Improved error handling in syncback_dir_no_meta to log and skip children with duplicate names in the file system.
This commit is contained in:
parent
06457bc0db
commit
4a09689090
3 changed files with 191 additions and 18 deletions
|
|
@ -15,6 +15,32 @@ local function isEmpty(table)
|
||||||
return next(table) == nil
|
return next(table) == nil
|
||||||
end
|
end
|
||||||
|
|
||||||
|
-- Finds duplicate names among children and returns a set of names that are duplicated
|
||||||
|
local function findDuplicateNames(children: { Instance } | { string }, virtualInstances: { [string]: any }?): { [string]: boolean }
|
||||||
|
local nameCounts: { [string]: number } = {}
|
||||||
|
local duplicates: { [string]: boolean } = {}
|
||||||
|
|
||||||
|
for _, child in children do
|
||||||
|
local name: string
|
||||||
|
if typeof(child) == "Instance" then
|
||||||
|
name = child.Name
|
||||||
|
elseif virtualInstances then
|
||||||
|
-- child is a virtual instance ID
|
||||||
|
local virtualChild = virtualInstances[child]
|
||||||
|
name = virtualChild and virtualChild.Name or ""
|
||||||
|
else
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
|
nameCounts[name] = (nameCounts[name] or 0) + 1
|
||||||
|
if nameCounts[name] > 1 then
|
||||||
|
duplicates[name] = true
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
return duplicates
|
||||||
|
end
|
||||||
|
|
||||||
local function fuzzyEq(a: number, b: number, epsilon: number): boolean
|
local function fuzzyEq(a: number, b: number, epsilon: number): boolean
|
||||||
return math.abs(a - b) < epsilon
|
return math.abs(a - b) < epsilon
|
||||||
end
|
end
|
||||||
|
|
@ -119,9 +145,86 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
updated = {},
|
updated = {},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
-- Count of locations with duplicate-named siblings
|
||||||
|
local skippedDuplicateCount = 0
|
||||||
|
|
||||||
|
-- Pre-scan to find ALL instances that are in an ambiguous path
|
||||||
|
-- (i.e., they or any of their ancestors have duplicate-named siblings)
|
||||||
|
local ambiguousIds: { [string]: boolean } = {}
|
||||||
|
|
||||||
|
local function markSubtreeAmbiguous(id)
|
||||||
|
if ambiguousIds[id] then
|
||||||
|
return
|
||||||
|
end
|
||||||
|
ambiguousIds[id] = true
|
||||||
|
|
||||||
|
local virtualInstance = virtualInstances[id]
|
||||||
|
if virtualInstance then
|
||||||
|
for _, childId in ipairs(virtualInstance.Children) do
|
||||||
|
markSubtreeAmbiguous(childId)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
-- Recursively scan for duplicates and mark ambiguous subtrees
|
||||||
|
local function scanForDuplicates(id)
|
||||||
|
local virtualInstance = virtualInstances[id]
|
||||||
|
if not virtualInstance then
|
||||||
|
return
|
||||||
|
end
|
||||||
|
|
||||||
|
local instance = instanceMap.fromIds[id]
|
||||||
|
|
||||||
|
-- Detect duplicate names among real DOM children (if instance exists)
|
||||||
|
local realDuplicates: { [string]: boolean } = {}
|
||||||
|
if instance then
|
||||||
|
local realChildren = instance:GetChildren()
|
||||||
|
realDuplicates = findDuplicateNames(realChildren)
|
||||||
|
|
||||||
|
-- Count locations with duplicates
|
||||||
|
if next(realDuplicates) then
|
||||||
|
skippedDuplicateCount += 1
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
-- Detect duplicate names among virtual DOM children
|
||||||
|
local virtualDuplicates = findDuplicateNames(virtualInstance.Children, virtualInstances)
|
||||||
|
|
||||||
|
-- Merge duplicates from both sides
|
||||||
|
local allDuplicates: { [string]: boolean } = {}
|
||||||
|
for name in realDuplicates do
|
||||||
|
allDuplicates[name] = true
|
||||||
|
end
|
||||||
|
for name in virtualDuplicates do
|
||||||
|
allDuplicates[name] = true
|
||||||
|
end
|
||||||
|
|
||||||
|
-- Mark all children with duplicate names (and their entire subtrees) as ambiguous
|
||||||
|
for _, childId in ipairs(virtualInstance.Children) do
|
||||||
|
local virtualChild = virtualInstances[childId]
|
||||||
|
if virtualChild and allDuplicates[virtualChild.Name] then
|
||||||
|
markSubtreeAmbiguous(childId)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
-- Recurse into non-ambiguous children to find deeper duplicates
|
||||||
|
for _, childId in ipairs(virtualInstance.Children) do
|
||||||
|
if not ambiguousIds[childId] then
|
||||||
|
scanForDuplicates(childId)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
-- First pass: scan entire tree to find all ambiguous paths
|
||||||
|
scanForDuplicates(rootId)
|
||||||
|
|
||||||
-- Add a virtual instance and all of its descendants to the patch, marked as
|
-- Add a virtual instance and all of its descendants to the patch, marked as
|
||||||
-- being added.
|
-- being added. Skip ambiguous instances.
|
||||||
local function markIdAdded(id)
|
local function markIdAdded(id)
|
||||||
|
if ambiguousIds[id] then
|
||||||
|
return
|
||||||
|
end
|
||||||
|
|
||||||
local virtualInstance = virtualInstances[id]
|
local virtualInstance = virtualInstances[id]
|
||||||
patch.added[id] = virtualInstance
|
patch.added[id] = virtualInstance
|
||||||
|
|
||||||
|
|
@ -132,6 +235,11 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
|
|
||||||
-- Internal recursive kernel for diffing an instance with the given ID.
|
-- Internal recursive kernel for diffing an instance with the given ID.
|
||||||
local function diffInternal(id)
|
local function diffInternal(id)
|
||||||
|
-- Skip entirely if this instance is in an ambiguous path
|
||||||
|
if ambiguousIds[id] then
|
||||||
|
return true
|
||||||
|
end
|
||||||
|
|
||||||
local virtualInstance = virtualInstances[id]
|
local virtualInstance = virtualInstances[id]
|
||||||
local instance = instanceMap.fromIds[id]
|
local instance = instanceMap.fromIds[id]
|
||||||
|
|
||||||
|
|
@ -155,6 +263,11 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
|
|
||||||
local changedProperties = {}
|
local changedProperties = {}
|
||||||
for propertyName, virtualValue in pairs(virtualInstance.Properties) do
|
for propertyName, virtualValue in pairs(virtualInstance.Properties) do
|
||||||
|
-- Skip CanvasPosition on ScrollingFrame (runtime state, not meaningful for sync)
|
||||||
|
if propertyName == "CanvasPosition" and virtualInstance.ClassName == "ScrollingFrame" then
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName)
|
local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName)
|
||||||
|
|
||||||
if getProperySuccess then
|
if getProperySuccess then
|
||||||
|
|
@ -222,8 +335,18 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
-- corresponding virtual instance should be removed. Any instance that
|
-- corresponding virtual instance should be removed. Any instance that
|
||||||
-- does have a corresponding virtual instance is recursively diffed.
|
-- does have a corresponding virtual instance is recursively diffed.
|
||||||
for _, childInstance in ipairs(instance:GetChildren()) do
|
for _, childInstance in ipairs(instance:GetChildren()) do
|
||||||
|
-- Skip TouchInterest (auto-created by Roblox when touch events are connected)
|
||||||
|
if childInstance.ClassName == "TouchInterest" then
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
local childId = instanceMap.fromInstances[childInstance]
|
local childId = instanceMap.fromInstances[childInstance]
|
||||||
|
|
||||||
|
-- Skip children in ambiguous paths
|
||||||
|
if childId and ambiguousIds[childId] then
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
if childId == nil then
|
if childId == nil then
|
||||||
-- pcall to avoid security permission errors
|
-- pcall to avoid security permission errors
|
||||||
local success, skip = pcall(function()
|
local success, skip = pcall(function()
|
||||||
|
|
@ -235,6 +358,14 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
continue
|
continue
|
||||||
end
|
end
|
||||||
|
|
||||||
|
-- Check if this unmapped child has a duplicate name among siblings
|
||||||
|
-- If so, skip it (we can't reliably determine what to do with it)
|
||||||
|
local siblings = instance:GetChildren()
|
||||||
|
local siblingDuplicates = findDuplicateNames(siblings)
|
||||||
|
if siblingDuplicates[childInstance.Name] then
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
-- This is an existing instance not present in the virtual DOM.
|
-- This is an existing instance not present in the virtual DOM.
|
||||||
-- We can mark it for deletion unless the user has asked us not
|
-- We can mark it for deletion unless the user has asked us not
|
||||||
-- to delete unknown stuff.
|
-- to delete unknown stuff.
|
||||||
|
|
@ -253,6 +384,11 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
-- Traverse the list of children in the virtual DOM. Any virtual
|
-- Traverse the list of children in the virtual DOM. Any virtual
|
||||||
-- instance that has no corresponding real instance should be created.
|
-- instance that has no corresponding real instance should be created.
|
||||||
for _, childId in ipairs(virtualInstance.Children) do
|
for _, childId in ipairs(virtualInstance.Children) do
|
||||||
|
-- Skip children in ambiguous paths
|
||||||
|
if ambiguousIds[childId] then
|
||||||
|
continue
|
||||||
|
end
|
||||||
|
|
||||||
local childInstance = instanceMap.fromIds[childId]
|
local childInstance = instanceMap.fromIds[childId]
|
||||||
|
|
||||||
if childInstance == nil then
|
if childInstance == nil then
|
||||||
|
|
@ -267,6 +403,14 @@ local function diff(instanceMap, virtualInstances, rootId)
|
||||||
|
|
||||||
local diffSuccess, err = diffInternal(rootId)
|
local diffSuccess, err = diffInternal(rootId)
|
||||||
|
|
||||||
|
-- Log count of skipped duplicates
|
||||||
|
if skippedDuplicateCount > 0 then
|
||||||
|
Log.warn(
|
||||||
|
"Skipped {} location(s) with duplicate-named siblings (cannot reliably sync)",
|
||||||
|
skippedDuplicateCount
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
if not diffSuccess then
|
if not diffSuccess then
|
||||||
return false, err
|
return false, err
|
||||||
end
|
end
|
||||||
|
|
|
||||||
|
|
@ -18,7 +18,8 @@ end
|
||||||
|
|
||||||
-- Counts lines that differ between two strings (capped at 100)
|
-- Counts lines that differ between two strings (capped at 100)
|
||||||
-- Returns: totalDiff, whitespaceDiff (lines that only differ in whitespace)
|
-- Returns: totalDiff, whitespaceDiff (lines that only differ in whitespace)
|
||||||
-- Returns 10000, 10000 if exceeds 100 (frontend displays as "100+")
|
-- Returns 10000 for totalDiff if exceeds 100 (frontend displays as "100+")
|
||||||
|
-- whitespaceDiff is 10000 only if ALL checked differences were whitespace-only
|
||||||
function WhitespaceUtil.CountLineDifferences(current: string, incoming: string): (number, number)
|
function WhitespaceUtil.CountLineDifferences(current: string, incoming: string): (number, number)
|
||||||
if type(current) ~= "string" or type(incoming) ~= "string" then
|
if type(current) ~= "string" or type(incoming) ~= "string" then
|
||||||
return 0, 0
|
return 0, 0
|
||||||
|
|
@ -29,6 +30,7 @@ function WhitespaceUtil.CountLineDifferences(current: string, incoming: string):
|
||||||
local maxLines = math.max(#currentLines, #incomingLines)
|
local maxLines = math.max(#currentLines, #incomingLines)
|
||||||
|
|
||||||
local totalDiff, whitespaceDiff = 0, 0
|
local totalDiff, whitespaceDiff = 0, 0
|
||||||
|
local hasContentChange = false
|
||||||
for i = 1, maxLines do
|
for i = 1, maxLines do
|
||||||
local currentLine = currentLines[i] or ""
|
local currentLine = currentLines[i] or ""
|
||||||
local incomingLine = incomingLines[i] or ""
|
local incomingLine = incomingLines[i] or ""
|
||||||
|
|
@ -37,9 +39,13 @@ function WhitespaceUtil.CountLineDifferences(current: string, incoming: string):
|
||||||
totalDiff += 1
|
totalDiff += 1
|
||||||
if NormalizeLine(currentLine) == NormalizeLine(incomingLine) then
|
if NormalizeLine(currentLine) == NormalizeLine(incomingLine) then
|
||||||
whitespaceDiff += 1
|
whitespaceDiff += 1
|
||||||
|
else
|
||||||
|
hasContentChange = true
|
||||||
end
|
end
|
||||||
if totalDiff > 100 then
|
if totalDiff > 100 then
|
||||||
return 10000, 10000
|
-- Return 10000 for both only if ALL differences were whitespace-only
|
||||||
|
-- Otherwise return different values so isWhitespaceOnly is false
|
||||||
|
return 10000, if hasContentChange then 0 else 10000
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
||||||
|
|
@ -134,24 +134,30 @@ pub fn syncback_dir_no_meta<'sync>(
|
||||||
let mut children = Vec::new();
|
let mut children = Vec::new();
|
||||||
let mut removed_children = Vec::new();
|
let mut removed_children = Vec::new();
|
||||||
|
|
||||||
// We have to enforce unique child names for the file system.
|
// Detect duplicate child names (case-insensitive for file system safety).
|
||||||
let mut child_names = HashSet::with_capacity(new_inst.children().len());
|
// We skip duplicates instead of failing, logging them for the user.
|
||||||
let mut duplicate_set = HashSet::new();
|
let mut child_name_counts: HashMap<String, usize> = HashMap::new();
|
||||||
for child_ref in new_inst.children() {
|
for child_ref in new_inst.children() {
|
||||||
let child = snapshot.get_new_instance(*child_ref).unwrap();
|
let child = snapshot.get_new_instance(*child_ref).unwrap();
|
||||||
if !child_names.insert(child.name.to_lowercase()) {
|
let lower_name = child.name.to_lowercase();
|
||||||
duplicate_set.insert(child.name.as_str());
|
*child_name_counts.entry(lower_name).or_insert(0) += 1;
|
||||||
}
|
}
|
||||||
}
|
|
||||||
if !duplicate_set.is_empty() {
|
let duplicate_names: HashSet<String> = child_name_counts
|
||||||
if duplicate_set.len() <= 25 {
|
.into_iter()
|
||||||
anyhow::bail!(
|
.filter(|(_, count)| *count > 1)
|
||||||
"Instance has children with duplicate name (case may not exactly match):\n {}",
|
.map(|(name, _)| name)
|
||||||
duplicate_set.into_iter().collect::<Vec<&str>>().join(", ")
|
.collect();
|
||||||
|
|
||||||
|
if !duplicate_names.is_empty() {
|
||||||
|
let inst_path = crate::syncback::inst_path(snapshot.new_tree(), snapshot.new);
|
||||||
|
let duplicate_list: Vec<&str> = duplicate_names.iter().map(|s| s.as_str()).collect();
|
||||||
|
log::warn!(
|
||||||
|
"Skipping children with duplicate names at '{}' (cannot reliably sync): {:?}",
|
||||||
|
inst_path,
|
||||||
|
duplicate_list
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
anyhow::bail!("Instance has more than 25 children with duplicate names");
|
|
||||||
}
|
|
||||||
|
|
||||||
if let Some(old_inst) = snapshot.old_inst() {
|
if let Some(old_inst) = snapshot.old_inst() {
|
||||||
let mut old_child_map = HashMap::with_capacity(old_inst.children().len());
|
let mut old_child_map = HashMap::with_capacity(old_inst.children().len());
|
||||||
|
|
@ -162,6 +168,13 @@ pub fn syncback_dir_no_meta<'sync>(
|
||||||
|
|
||||||
for new_child_ref in new_inst.children() {
|
for new_child_ref in new_inst.children() {
|
||||||
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
||||||
|
|
||||||
|
// Skip children with duplicate names - cannot reliably sync them
|
||||||
|
if duplicate_names.contains(&new_child.name.to_lowercase()) {
|
||||||
|
old_child_map.remove(new_child.name.as_str());
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
// Skip instances of ignored classes
|
// Skip instances of ignored classes
|
||||||
if snapshot.should_ignore_class(&new_child.class) {
|
if snapshot.should_ignore_class(&new_child.class) {
|
||||||
// Also remove from old_child_map so it won't be marked as removed
|
// Also remove from old_child_map so it won't be marked as removed
|
||||||
|
|
@ -203,8 +216,12 @@ pub fn syncback_dir_no_meta<'sync>(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Any children that are in the old dom but not the new one are removed.
|
// Any children that are in the old dom but not the new one are removed.
|
||||||
// Filter out instances of ignored classes from removal.
|
// Filter out instances of ignored classes and duplicates from removal.
|
||||||
removed_children.extend(old_child_map.into_values().filter(|inst| {
|
removed_children.extend(old_child_map.into_values().filter(|inst| {
|
||||||
|
// Don't remove duplicates - we're skipping them entirely
|
||||||
|
if duplicate_names.contains(&inst.name().to_lowercase()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
if snapshot.should_ignore_class(inst.class_name().as_str()) {
|
if snapshot.should_ignore_class(inst.class_name().as_str()) {
|
||||||
log::debug!(
|
log::debug!(
|
||||||
"Not removing instance {} because its class {} is ignored",
|
"Not removing instance {} because its class {} is ignored",
|
||||||
|
|
@ -220,6 +237,12 @@ pub fn syncback_dir_no_meta<'sync>(
|
||||||
// There is no old instance. Just add every child.
|
// There is no old instance. Just add every child.
|
||||||
for new_child_ref in new_inst.children() {
|
for new_child_ref in new_inst.children() {
|
||||||
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
|
||||||
|
|
||||||
|
// Skip children with duplicate names - cannot reliably sync them
|
||||||
|
if duplicate_names.contains(&new_child.name.to_lowercase()) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
// Skip instances of ignored classes
|
// Skip instances of ignored classes
|
||||||
if snapshot.should_ignore_class(&new_child.class) {
|
if snapshot.should_ignore_class(&new_child.class) {
|
||||||
log::debug!(
|
log::debug!(
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue