Skip to content

Conversation

sjoerdvisscher
Copy link
Member

@sjoerdvisscher sjoerdvisscher commented Dec 11, 2019

Fixes #257

@sjoerdvisscher sjoerdvisscher requested a review from epost December 11, 2019 12:43
@sjoerdvisscher
Copy link
Member Author

@epost can this be merged?

@sjoerdvisscher
Copy link
Member Author

Oh right, this is superseded by #296

@epost
Copy link
Member

epost commented Dec 16, 2019

No not yet, I have a pending comments/WIP notes to myself on this PR.

@epost epost reopened this Dec 16, 2019
Copy link
Member

@epost epost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments, which now apply to #296.

@@ -15,6 +15,7 @@
<link rel="stylesheet" href="diagram.css">
<link rel="stylesheet" href="typedefs.css">
<link rel="stylesheet" href="auths.css">
<link rel="stylesheet" href="bricks.css">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add comments on what these CSS files are needed for, in particular as things like 'bricks' vs' 'diagram' aren't terribly instructive.

@@ -42,11 +47,13 @@ type ChildSlots =
( objectTree :: H.Slot VoidF (TreeMenu.Msg Route) Unit
, petrinetEditor :: H.Slot VoidF PetrinetEditor.Msg Unit
, diagramEditor :: H.Slot VoidF DiagramEditor.Msg Unit
, bricks :: Bricks.Slot Unit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think this should perhaps be called kdmoncatBricks or something, with the goal of clarifying what sort of bricks we're talking about.
  2. Perhaps we could qualify Bricks as KDMonCat.Bricks or something.

)

_objectTree = SProxy :: SProxy "objectTree"
_petrinetEditor = SProxy :: SProxy "petrinetEditor"
_diagramEditor = SProxy :: SProxy "diagramEditor"
_bricks = SProxy :: SProxy "bricks"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, _kdmoncat, or _kdmoncatBricks or something?

@@ -40,6 +41,19 @@ fromDiagram { width, pixels, names } = fromEdges (_ - 1) name edges
edges = concat $ zipWith (zipWith (\src tgt -> { src, tgt })) rows (drop 1 rows)
name id = names !! (id - 1) # fromMaybe "?"

type Op r = { label :: String, pos :: Vec3 Int | r }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This (more or less) duplicates Operator from halogen-diagram-editor. Should we work with that one somehow instead of redefining it here?

  • Let's add a newline after the definition plz, in keeping with the rest of this codebase. 🙏

@@ -40,6 +41,19 @@ fromDiagram { width, pixels, names } = fromEdges (_ - 1) name edges
edges = concat $ zipWith (zipWith (\src tgt -> { src, tgt })) rows (drop 1 rows)
name id = names !! (id - 1) # fromMaybe "?"

type Op r = { label :: String, pos :: Vec3 Int | r }
fromOps :: ∀ r. Array (Op r) -> DiagramV2
fromOps ops = fromEdges identity ((ops !! _) >>> maybe "" _.label) edges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'Op(s)' name is a holdover from the Elm code. it's used there internally in places, but:

  • The type itself is called Operator, written out in full.
  • This name should change, see also nomenclature document.
  • Especially outside of halogen-diagram-editor, let's try to be a bit explicit; Operator, perhaps even a fully qualified. for local variable names, i guess it's ok to use op until we decide on a better name, as long as some nearby type indicates that it means Operator.

where
edges = ops # foldMapWithIndex \src { pos : srcPos } ->
ops # foldMapWithIndex \tgt { pos : tgtPos } ->
if _y tgtPos == _y srcPos + 1 then let
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduce a helper along the lines of isUnderneath (or whatever it is that this does)?

tgtStart = _x tgtPos
srcEnd = srcStart + _z srcPos
tgtEnd = tgtStart + _z tgtPos
in if srcStart < tgtEnd && tgtStart < srcEnd then [{ src, tgt }] else [] else []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rewrite this as where clauses?

@epost
Copy link
Member

epost commented Dec 17, 2019

Superseded by #296.

@epost epost closed this Dec 17, 2019
@sjoerdvisscher sjoerdvisscher deleted the kdmoncat branch December 18, 2019 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

studio: View wirings using kdmoncat brick diagram editor
2 participants