Skip to content

Commit

Permalink
#5643: More work on visualising conflicts, add some handling code to …
Browse files Browse the repository at this point in the history 
…EntityInspector.
  • Loading branch information
@codereader
codereader committed Jun 19, 2021
1 parent 2d21b62 commit 16cd7ea
Show file tree
Hide file tree
Showing 9 changed files with 107 additions and 29 deletions.
16 changes: 11 additions & 5 deletions include/imapmerge.h
View file
Expand Up @@ -44,6 +44,13 @@ enum class ConflictType
SettingKeyToDifferentValue,
};

enum class ResolutionType
{
Unresolved,
RejectSourceChange,
ApplySourceChange,
};

/**
* Represents a merge action, i.e. one single step of a merge operation.
* Only active actions will be processed when the merge run starts.
Expand Down Expand Up @@ -113,12 +120,11 @@ class IConflictResolutionAction :
// The affected entity node
virtual const INodePtr& getConflictingEntity() const = 0;

// Whether this action has been resolved by importing the source change over the target
virtual bool isResolvedByUsingSource() const = 0;
// Whether this action has been resolved at all, and what has been chosen
virtual ResolutionType getResolution() const = 0;

// Resolve this action by accepting the source changes, causing the to overwrite the
// conflicting change in the target map.
virtual void setResolvedByUsingSource(bool applySourceChange) = 0;
// Resolve this action by either accepting or rejecting the source change
virtual void setResolution(ResolutionType resolution) = 0;
};

// A MergeOperation groups one or more merge actions
Expand Down
6 changes: 3 additions & 3 deletions install/ui/mergecontroldialog.fbp
View file
Expand Up @@ -1683,7 +1683,7 @@
<property name="minimize_button">0</property>
<property name="minimum_size"></property>
<property name="moveable">1</property>
<property name="name">ChangeConflicts</property>
<property name="name">UnresolvedConflicts</property>
<property name="pane_border">1</property>
<property name="pane_position"></property>
<property name="pane_size"></property>
Expand Down Expand Up @@ -1735,7 +1735,7 @@
<property name="gripper">0</property>
<property name="hidden">0</property>
<property name="id">wxID_ANY</property>
<property name="label">Change Conflicts</property>
<property name="label">Unresolved Conflicts</property>
<property name="markup">0</property>
<property name="max_size"></property>
<property name="maximize_button">0</property>
Expand All @@ -1744,7 +1744,7 @@
<property name="minimize_button">0</property>
<property name="minimum_size"></property>
<property name="moveable">1</property>
<property name="name">m_staticText221</property>
<property name="name">UnresolvedConflictsLabel</property>
<property name="pane_border">1</property>
<property name="pane_position"></property>
<property name="pane_size"></property>
Expand Down
6 changes: 3 additions & 3 deletions install/ui/mergecontroldialog.xrc
View file
Expand Up @@ -335,7 +335,7 @@
<option>0</option>
<flag>wxALL</flag>
<border>5</border>
<object class="wxStaticText" name="ChangeConflicts">
<object class="wxStaticText" name="UnresolvedConflicts">
<font>
<style>normal</style>
<weight>bold</weight>
Expand All @@ -349,8 +349,8 @@
<option>0</option>
<flag>wxALL</flag>
<border>5</border>
<object class="wxStaticText" name="m_staticText221">
<label>Change Conflicts</label>
<object class="wxStaticText" name="UnresolvedConflictsLabel">
<label>Unresolved Conflicts</label>
<wrap>-1</wrap>
</object>
</object>
Expand Down
14 changes: 7 additions & 7 deletions libs/scene/merge/MergeAction.h
View file
Expand Up @@ -289,7 +289,7 @@ class ConflictResolutionAction :
// The action that happened in the target
MergeAction::Ptr _targetAction;

bool _applySourceChange;
ResolutionType _resolution;

protected:
ConflictResolutionAction(ConflictType conflictType, const INodePtr& conflictingEntity, const MergeAction::Ptr& sourceAction) :
Expand All @@ -302,7 +302,7 @@ class ConflictResolutionAction :
_conflictingEntity(conflictingEntity),
_sourceAction(sourceAction),
_targetAction(targetAction),
_applySourceChange(false)
_resolution(ResolutionType::Unresolved)
{}

public:
Expand Down Expand Up @@ -335,21 +335,21 @@ class ConflictResolutionAction :
return getConflictingEntity();
}

bool isResolvedByUsingSource() const
ResolutionType getResolution() const override
{
return _applySourceChange;
return _resolution;
}

void setResolvedByUsingSource(bool applySourceChange)
void setResolution(ResolutionType resolution) override
{
_applySourceChange = applySourceChange;
_resolution = resolution;
}

void applyChanges() override
{
if (!isActive()) return;

if (_applySourceChange)
if (_resolution == ResolutionType::ApplySourceChange)
{
_sourceAction->applyChanges();
}
Expand Down
13 changes: 13 additions & 0 deletions libs/wxutil/dataview/TreeViewItemStyle.h
View file
Expand Up @@ -88,6 +88,19 @@ class TreeViewItemStyle
SetStrikethrough(attr, true);
}

static void ApplyKeyValueConflictStyle(wxDataViewItemAttr& attr)
{
if (SupportsBackgroundColour())
{
SetBackgroundColour(attr, wxColour(89, 255, 0));
}
else
{
attr.SetColour(wxColour(89, 255, 0));
attr.SetBold(true);
}
}

static void SetStrikethrough(wxDataViewItemAttr& attr, bool enabled)
{
#if wxCHECK_VERSION(3, 1, 2)
Expand Down
32 changes: 30 additions & 2 deletions radiant/ui/einspector/EntityInspector.cpp
View file
Expand Up @@ -241,6 +241,12 @@ void EntityInspector::onKeyChange(const std::string& key,
}
}

// Conflicting actions get a special render style
if (_conflictActions.count(key) > 0)
{
wxutil::TreeViewItemStyle::ApplyKeyValueConflictStyle(style);
}

wxIcon icon;
icon.CopyFromBitmap(parms.type.empty() ? _emptyIcon : PropertyEditorFactory::getBitmapFor(parms.type));

Expand Down Expand Up @@ -408,6 +414,7 @@ void EntityInspector::onMainFrameConstructed()
void EntityInspector::onMainFrameShuttingDown()
{
_mergeActions.clear();
_conflictActions.clear();

_undoHandler.disconnect();
_redoHandler.disconnect();
Expand Down Expand Up @@ -957,6 +964,13 @@ void EntityInspector::_onRejectMergeAction()

auto key = row[_columns.name].getString().ToStdString();

auto conflict = _conflictActions.find(key);

if (conflict != _conflictActions.end())
{
conflict->second->setResolution(scene::merge::ResolutionType::RejectSourceChange);
}

auto action = _mergeActions.find(key);

if (action != _mergeActions.end())
Expand Down Expand Up @@ -996,7 +1010,7 @@ bool EntityInspector::_testRejectMergeAction()
bool EntityInspector::isItemAffecedByMergeOperation(const wxutil::TreeModel::Row& row)
{
auto key = row[_columns.name].getString().ToStdString();
return _mergeActions.count(key) > 0;
return _mergeActions.count(key) > 0 || _conflictActions.count(key) > 0;
}

// wxWidget callbacks
Expand Down Expand Up @@ -1418,6 +1432,7 @@ void EntityInspector::changeSelectedEntity(const scene::INodePtr& newEntity, con
_keyValueIterMap.clear();
_kvStore->Clear();
_mergeActions.clear();
_conflictActions.clear();

// Reset the sorting when changing entities
_keyValueTreeView->ResetSortingOnAllColumns();
Expand Down Expand Up @@ -1472,7 +1487,20 @@ void EntityInspector::handleMergeActions(const scene::INodePtr& selectedNode)
}
}

// TODO: Conflict resolution actions
auto conflictAction = std::dynamic_pointer_cast<scene::merge::IConflictResolutionAction>(action);

if (conflictAction)
{
auto sourceAction = std::dynamic_pointer_cast<scene::merge::IEntityKeyValueMergeAction>(conflictAction->getSourceAction());

if (sourceAction)
{
// The source action will be rendered as change
_mergeActions[sourceAction->getKey()] = sourceAction;
// Remember the conflict action too
_conflictActions[sourceAction->getKey()] = conflictAction;
}
}
});
}

Expand Down
1 change: 1 addition & 0 deletions radiant/ui/einspector/EntityInspector.h
View file
Expand Up @@ -149,6 +149,7 @@ class EntityInspector :

// Maps the key names to a possible merge action that should be displayed
std::map<std::string, scene::merge::IEntityKeyValueMergeAction::Ptr> _mergeActions;
std::map<std::string, scene::merge::IConflictResolutionAction::Ptr> _conflictActions;

private:
bool canUpdateEntity();
Expand Down
36 changes: 33 additions & 3 deletions radiant/ui/merge/MergeControlDialog.cpp
View file
Expand Up @@ -317,7 +317,7 @@ void MergeControlDialog::updateSummary()
std::size_t entitiesRemoved = 0;
std::size_t primitivesAdded = 0;
std::size_t primitivesRemoved = 0;
std::size_t changeConflicts = 0;
std::size_t unresolvedConflicts = 0;

if (operation)
{
Expand Down Expand Up @@ -347,9 +347,16 @@ void MergeControlDialog::updateSummary()
modifiedEntities.insert(action->getAffectedNode());
break;
case scene::merge::ActionType::ConflictResolution:
changeConflicts++;
{
auto conflict = std::dynamic_pointer_cast<scene::merge::IConflictResolutionAction>(action);

if (conflict && conflict->getResolution() == scene::merge::ResolutionType::Unresolved)
{
unresolvedConflicts++;
}
break;
}
}
});

entitiesModified = modifiedEntities.size();
Expand All @@ -360,7 +367,30 @@ void MergeControlDialog::updateSummary()
findNamedObject<wxStaticText>(this, "EntitiesModified")->SetLabel(string::to_string(entitiesModified));
findNamedObject<wxStaticText>(this, "PrimitivesAdded")->SetLabel(string::to_string(primitivesAdded));
findNamedObject<wxStaticText>(this, "PrimitivesRemoved")->SetLabel(string::to_string(primitivesRemoved));
findNamedObject<wxStaticText>(this, "ChangeConflicts")->SetLabel(string::to_string(changeConflicts));
findNamedObject<wxStaticText>(this, "UnresolvedConflicts")->SetLabel(string::to_string(unresolvedConflicts));

if (unresolvedConflicts > 0)
{
makeLabelBold(this, "UnresolvedConflicts");
makeLabelBold(this, "UnresolvedConflictsLabel");

findNamedObject<wxStaticText>(this, "UnresolvedConflicts")->SetForegroundColour(wxColour(200, 0, 0));
findNamedObject<wxStaticText>(this, "UnresolvedConflictsLabel")->SetForegroundColour(wxColour(200, 0, 0));
}
else
{
auto label = findNamedObject<wxStaticText>(this, "UnresolvedConflicts");
auto font = label->GetFont();
font.SetWeight(wxFONTWEIGHT_NORMAL);
label->SetFont(font);
label->SetForegroundColour(wxNullColour);

label = findNamedObject<wxStaticText>(this, "UnresolvedConflictsLabel");
font = label->GetFont();
font.SetWeight(wxFONTWEIGHT_NORMAL);
label->SetFont(font);
label->SetForegroundColour(wxNullColour);
}

Layout();
Fit();
Expand Down
12 changes: 6 additions & 6 deletions test/MapMerging.cpp
View file
Expand Up @@ -2194,7 +2194,7 @@ TEST_F(ThreeWayMergeTest, MergeEntityNameCollisions)
EXPECT_EQ(Node_getEntity(node_3)->getKeyValue("target0"), "node_4");

// Actively resolve the conflict and apply the change
keyValueConflict->setResolvedByUsingSource(true);
keyValueConflict->setResolution(ResolutionType::ApplySourceChange);
keyValueConflict->applyChanges();

node_3 = algorithm::getEntityByName(operation->getTargetRoot(), "node_3");
Expand Down Expand Up @@ -2223,7 +2223,7 @@ TEST_F(ThreeWayMergeTest, RemovalOfModifiedEntity)
EXPECT_TRUE(algorithm::getEntityByName(operation->getTargetRoot(), "func_static_8"));

// Now accept the change explicitly
entityConflict->setResolvedByUsingSource(true);
entityConflict->setResolution(ResolutionType::ApplySourceChange);
entityConflict->applyChanges();

// The entity func_static_8 has now been removed in target
Expand Down Expand Up @@ -2251,7 +2251,7 @@ TEST_F(ThreeWayMergeTest, ModificationOfRemovedEntity)
EXPECT_FALSE(algorithm::getEntityByName(operation->getTargetRoot(), "light_1"));

// Now accept the change explicitly
entityConflict->setResolvedByUsingSource(true);
entityConflict->setResolution(ResolutionType::ApplySourceChange);
entityConflict->applyChanges();

// The entity light_1 has now been imported to target
Expand Down Expand Up @@ -2289,7 +2289,7 @@ TEST_F(ThreeWayMergeTest, SettingKeyValueToConflictingValues)
EXPECT_EQ(Node_getEntity(entity)->getKeyValue("origin"), "224 200 32");

// Now accept the change explicitly
valueConflict->setResolvedByUsingSource(true);
valueConflict->setResolution(ResolutionType::ApplySourceChange);
valueConflict->applyChanges();

// The changed value has now been imported
Expand Down Expand Up @@ -2330,7 +2330,7 @@ TEST_F(ThreeWayMergeTest, ModificationOfRemovedKeyValue)
EXPECT_EQ(Node_getEntity(entity)->getKeyValue("extra3"), "");

// Now accept the change explicitly
valueConflict->setResolvedByUsingSource(true);
valueConflict->setResolution(ResolutionType::ApplySourceChange);
valueConflict->applyChanges();

// The changed value has now been imported
Expand Down Expand Up @@ -2371,7 +2371,7 @@ TEST_F(ThreeWayMergeTest, RemovalOfModifiedKeyValue)
EXPECT_EQ(Node_getEntity(entity)->getKeyValue("extra2"), "value2_changed");

// Now accept the change explicitly
valueConflict->setResolvedByUsingSource(true);
valueConflict->setResolution(ResolutionType::ApplySourceChange);
valueConflict->applyChanges();

// The changed value has now been removed
Expand Down

0 comments on commit 16cd7ea

Please sign in to comment.