Skip to content

Commit

Permalink
added attribute removal when setting an existing attribute on graphs,…
Browse files Browse the repository at this point in the history
… added more tests and went through operations which could fail and reasonably should error
  • Loading branch information
mpan322 committed Jun 4, 2024
1 parent a5d4697 commit 1893ed3
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 44 deletions.
57 changes: 28 additions & 29 deletions gap/dot.gi
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,9 @@ function(x, name, value)
" be removed using GraphvizRemoveAttr");
fi;

attrs := GraphvizAttrs(x);
name := String(name);
GV_RemoveGraphAttrIfExists(x, name);
attrs := GraphvizAttrs(x);
value := String(value);
if ' ' in value then
# Replace with call to GV_QuoteName or whatever TODO
Expand All @@ -291,9 +292,31 @@ end);
InstallMethod(GraphvizSetAttr, "for a graphviz (di)graph or context and object",
[IsGraphvizGraphDigraphOrContext, IsObject],
function(x, value)
local attrs;
attrs := GraphvizAttrs(x);
local attrs, match, pred;

match := function(lookup, target)
local idx, pred;
idx := 1;

pred := function(i)
return i <= Length(target) and i <= Length(lookup)
and lookup[i] = target[i] and lookup[i] <> '=';
end;

while pred(idx) do
idx := idx + 1;
od;
if idx > Length(lookup) or idx > Length(lookup) then
return false;
elif lookup[idx] = '=' and target[idx] = '=' then
return true;
fi;
return false;
end;

attrs := GraphvizAttrs(x);
x!.Attrs := Filtered(attrs, attr -> not match(attr, value));
attrs := GraphvizAttrs(x);
Add(attrs, String(value));
return x;
end);
Expand Down Expand Up @@ -564,35 +587,11 @@ InstallMethod(GraphvizRemoveAttr,
"for a graphviz (di)graph or context and an object",
[IsGraphvizGraphDigraphOrContext, IsObject],
function(obj, attr)
local attrs, i, match, len;
local attrs, len;
attrs := GraphvizAttrs(obj);
attr := String(attr);

# checks if they attribute names match the one being removed
match := function(key, str)
for i in [1 .. Length(key)] do
if i > Length(str) or key[i] <> str[i] then
return false;
fi;
od;

i := i + 1;
while i <= Length(str) do
if str[i] = '=' then
return true;
elif str[i] <> '\s' and str[i] <> '\t' then
return false;
fi;
i := i + 1;
od;

# attributes which are not key value or removal by value
return true;
end;

len := Length(attrs);
obj!.Attrs := Filtered(attrs, s -> not match(attr, s));

GV_RemoveGraphAttrIfExists(obj, attr);
# error if no attributes were removed i.e. did not exist
if Length(obj!.Attrs) - len = 0 then
ErrorFormatted("the 2nd argument (attribute name or attribute) \"{}\" ",
Expand Down
3 changes: 3 additions & 0 deletions gap/gv.gd
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ DeclareOperation("GV_AddEdge",
DeclareOperation("GV_GetIdx", [IsGraphvizObject]);
DeclareOperation("GV_ConstructHistory", [IsGraphvizGraphDigraphOrContext]);

DeclareOperation("GV_RemoveGraphAttrIfExists",
[IsGraphvizGraphDigraphOrContext, IsString]);

DeclareGlobalFunction("GV_IsValidColor");
DeclareGlobalFunction("GV_ErrorIfNotNodeColoring");
DeclareGlobalFunction("GV_ErrorIfNotValidLabel");
Expand Down
33 changes: 33 additions & 0 deletions gap/gv.gi
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,39 @@ function(x, edge)
return x;
end);

InstallMethod(GV_RemoveGraphAttrIfExists,
"for a graphviz graph context or digraph and a string",
[IsGraphvizGraphDigraphOrContext, IsString],
function(obj, attr)
local attrs, i, match;
attrs := GraphvizAttrs(obj);
attr := String(attr);

# checks if they attribute names match the one being removed
match := function(key, str)
for i in [1 .. Length(key)] do
if i > Length(str) or key[i] <> str[i] then
return false;
fi;
od;

i := i + 1;
while i <= Length(str) do
if str[i] = '=' then
return true;
elif str[i] <> '\s' and str[i] <> '\t' then
return false;
fi;
i := i + 1;
od;

# attributes which are not key value or removal by value
return true;
end;

obj!.Attrs := Filtered(attrs, s -> not match(attr, s));
end);

###############################################################################
# Stringifying
###############################################################################
Expand Down
20 changes: 14 additions & 6 deletions plan.md
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
## Possible Failure Cases:
- Add node
- if node exists with the same name it should fail. ``
- if the node's name is [not a valid node identifier](https://graphviz.org/doc/info/lang.html) it should fail immediately ``
- if the node's name is [not a valid node identifier](https://graphviz.org/doc/info/lang.html) it should fail immediately `X` (at the moment there is only very basic checking done when stringifying).
- Add edge
- if either of the nodes do not exist it automatically creates the node. I think this is fine as it is much more convienent. ``
- it should also allow for node failure cases above based on their names ``
- Remove node
- if the node does not exist it should fail.
- if the node does not exist it should fail ``
- Remove edge
- if no such edge exists it should fail
- if no such edge exists it should fail ``
- Add subgraph
- if there is already a subgraph with the same name within the same parent it should fail
- Remove subgraph
- if no such subgraph exists it should fail
- if there is already a subgraph with the same name within the same parent it should fail ``
- **QUESTION**: At the moment this check is only done within the parent i.e. if *g* has subgraphs *a* and *b* it is valid for both *a* and *b* to have children labelled *c*.
- I am parial to removing this as it will likely make the subgraph tree searching operations difficult to use properly or not make sense in their current version.
- If there is a good reason to keep this we can update such functions.
- Add attribute
- if no known attribute exists there is a warning (discussed this previously) ``
- Remove attribute
- removing an attribute which is not set causes in error ``
- Set attribute (which is already set)
- overwrites but does not change warn or error (I find this behaviour fine - let me know if you disagree) ``
- *CHANGE*: made it so adding an attribute to a graph now follows the above behaviour (before it was not removed)
2 changes: 1 addition & 1 deletion tst/dot.tst
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ gap> GraphvizAttrs(g);
[ "color=red", "shape=circle" ]
gap> GraphvizSetAttrs(g, rec(color := "blue", label := "test"));;
gap> GraphvizAttrs(g);
[ "color=red", "shape=circle", "label=test", "color=blue" ]
[ "shape=circle", "label=test", "color=blue" ]

# Test stringify
gap> g := GraphvizGraph();;
Expand Down
11 changes: 11 additions & 0 deletions tst/edge.tst
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,17 @@ gap> GraphvizEdges(g);
gap> GraphvizRemoveEdges(g, "c", "d");
Error, no edges exist from "d" to "c"

# Test filtering edges by names - nodes do not exist (digraph)
gap> g := GraphvizDigraph();;
gap> GraphvizRemoveEdges(g, "a", "b");
Error, no nodes with names "a" or "b"
gap> GraphvizAddNode(g, "a");;
gap> GraphvizRemoveEdges(g, "a", "b");
Error, no node with name "b"
gap> GraphvizAddNode(g, "b");;
gap> GraphvizRemoveEdges(g, "c", "b");
Error, no node with name "c"

# Test filtering edges by names (graph)
gap> g := GraphvizGraph();;
gap> a := GraphvizAddNode(g, "a");;
Expand Down
31 changes: 23 additions & 8 deletions tst/graph.tst
Original file line number Diff line number Diff line change
Expand Up @@ -195,19 +195,12 @@ gap> GraphvizSetAttr(g, "color", "red");;
gap> GraphvizAttrs(g);
[ "color=red" ]

# Test global attributes graph (duplicates)
gap> g := GraphvizGraph();;
gap> GraphvizSetAttr(g, "color", "red");;
gap> GraphvizSetAttr(g, "color", "blue");;
gap> GraphvizAttrs(g);
[ "color=red", "color=blue" ]

# Test stringify attributes graph
gap> g := GraphvizGraph();;
gap> GraphvizSetAttr(g, "color", "red");;
gap> GraphvizSetAttr(g, "color", "blue");;
gap> AsString(g);
"//dot\ngraph {\n\tcolor=red color=blue \n}\n"
"//dot\ngraph {\n\tcolor=blue \n}\n"

# # Test removing attributes from a graph TODO uncomment or delete
# gap> g := GraphvizGraph();;
Expand Down Expand Up @@ -280,5 +273,27 @@ gap> GraphvizSetAttr(g, "color", "red");;
gap> GraphvizAttrs(g);
[ "color=red" ]

# Test removing attributes which do not exist
gap> g := GraphvizGraph();;
gap> GraphvizRemoveAttr(g, "test");
Error, the 2nd argument (attribute name or attribute) "test" is not set on the\
provided object.
gap> n := GraphvizAddNode(g, "a");;
gap> GraphvizRemoveAttr(n, "test");
Error, the 2nd argument (attribute name) "test" is not set on the provided obj\
ect.

# Test overwriting attributes removes them
gap> g := GraphvizGraph();;
gap> GraphvizSetAttr(g, "color", "red");;
gap> GraphvizAttrs(g);
[ "color=red" ]
gap> GraphvizSetAttr(g, "color", "blue");;
gap> GraphvizAttrs(g);
[ "color=blue" ]
gap> GraphvizSetAttr(g, "color=green");;
gap> GraphvizAttrs(g);
[ "color=green" ]

#
gap> STOP_TEST("graphviz package: graph.tst", 0);
8 changes: 8 additions & 0 deletions tst/subgraph.tst
Original file line number Diff line number Diff line change
Expand Up @@ -470,5 +470,13 @@ gap> b;
gap> c;
<graphviz graph "3" with 4 nodes and 4 edges>

# adding subgraphs with the same name (nested)
gap> g := GraphvizGraph("g");;
gap> a := GraphvizAddSubgraph(g, "a");;
gap> b := GraphvizAddSubgraph(g, "b");;
gap> GraphvizAddSubgraph(a, "c");;
gap> GraphvizAddSubgraph(b, "c");
<graphviz graph "c" with 0 nodes and 0 edges>

#
gap> STOP_TEST("graphviz package: subgraph.tst", 0);

0 comments on commit 1893ed3

Please sign in to comment.