Multiple custom layouts for tracing views #3299

MichaelBuessemeyer · 2018-10-02T10:13:07Z

This PR addes the possibility to add muliple layouts.

URL of deployed dev instance (used for testing):

https://mulitpletracingcustomlayouts.webknossos.xyz/

Steps to test:

open a tracing
create a new layout (should have the previous layout set as initial layout)
change freely between the layouts
try deleting a non-active layout
an active layout should not be removable -> thus guaranteeing to always have a layout
the last active layout at closing a tracing, should be the initial layout on next startup
important -> try this in all four kinds of tracing views as their layout is treated separately
test the migration into the new "layout saving schema"

Issues:

fixes Pre-defined and/or customizable layout presets #3230

Updated changelog
Ready for review

…tive layout as state to action bar; reworked layout persistence to match new layout schema

… active layout is resetable

…new layout name

…nd adding

…itple-tracing-custom-layouts

MichaelBuessemeyer · 2018-10-02T10:18:48Z

@philippotto shall we / I add a short "feature explanation" to the documentation? Right now I can't find anything about the custom viewports / layout feature.

…minds/webknossos into mulitple-tracing-custom-layouts

philippotto

Wow! 👍 Awesome work! I just tested it and it seems to work pretty well. I left some review comments for the code and I also have some remarks for the UI:

the icons to delete a layout should be aligned to the far right in my opinion
can we add a way to edit the name of an existing layout?
we should make it more clear that we store different layouts for the different UI setups. One way would be to add an information icon which has some explanation text. Another idea would be to list all layouts in a categorized way, but disable the ones which can not be used.
E.g.:

---- Orthogonal Mode ----
Default Layout
Layout 1
Layout 2
---- Arbitrary Mode ------
Default Layout
Layout 1

(cursive is disabled).

What do you think? Maybe you can brainstorm a bit, too, so that we can discuss afterwards with which solution we want to go?

philippotto · 2018-10-02T13:55:28Z

app/assets/javascripts/oxalis/view/layouting/golden_layout_adapter.js

@@ -103,17 +106,16 @@ export class GoldenLayoutAdapter extends React.PureComponent<Props<*>, *> {
  }

  onStateChange() {
+    console.log("changing gl state");


This log can probably be removed.

philippotto · 2018-10-02T14:05:50Z

app/assets/javascripts/oxalis/view/layouting/layout_persistence.js

+
+export function setActiveLayout(layoutKey: LayoutKeys, activeLayout: string) {
+  const newLayouts = getDeepCopyOfStoredLayouts();
+  if (newLayouts[layoutKey] && newLayouts[layoutKey][activeLayout]) {


Shouldn't we throw an error if this if-check fails?

Like an error toast that says e.g. "The selected layout was not found. Please add a layout for the selected name."?

I was thinking of a simple js throw via throw new Error("Active layout could not be set ..."). This case should never occur, right? If there is no plausible way for the user, to hit this case, we do not need to add code here for a user-facing error message (so, no toast). But if the if-check shouldn't pass for some reason (e.g., future changes break something), then it would be very helpful to see an error in the console, when debugging this. That's why, you should usually just throw an error in these cases, since the error would be silently swallowed otherwise.

philippotto · 2018-10-02T14:06:59Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "webknossos",
-  "version": "0.17.146",
+  "version": "1.10.1",


I think this change was added when you executed yarn version. Can you undo this?

philippotto · 2018-10-02T14:15:40Z

app/assets/javascripts/oxalis/view/layouting/layout_persistence.js

+      if (version !== 5) {
+        return defaultLayoutSchema;
+      }
+      // migrate to newst schema


philippotto · 2018-10-02T14:20:51Z

app/assets/javascripts/oxalis/view/layouting/layout_persistence.js

+        },
+        OrthoLayout: {
+          "Custom Layout": layouts.OrthoLayout || defaultLayouts.OrthoLayout,
+          lastActive: "Custom Layout",


This lastActive attribute has some disadvantages in my opinion. Two example symptoms are that a layout must not be named "lastActive" and that "lastActive" has to be ignored when listing layout names. In general, the problem is that OrthoLayout (and co) contain a map from layout name to layout config AND the information what is active. These are two different things which can be stored independently.
For example, you could have:

... OrthoLayout: {...}, LastActiveLayouts: { OrthoLayout: "Custom Layout", ArbitraryLayout: "Layout X", }

as a format. Do you agree?

Sure it was obvious to me that my solution is not so good. I like your idea way more than mine and it makes the code even better 😃

…itple-tracing-custom-layouts

philippotto · 2018-10-05T09:25:45Z

app/assets/javascripts/oxalis/view/action-bar/tracing_actions_view.js

+    <React.Fragment>
+      <h5 style={{ color: "#fff" }}>Where is my layout?</h5>
+      <p>
+        {`The tracing views are seperated into four classes. Each of them has their own layouts. If you


separated

philippotto · 2018-10-05T09:26:39Z

app/assets/javascripts/oxalis/view/layouting/default_layout_configs.js

+  OrthoLayoutView: "Othogonal Tracings - View Only",
+  VolumeTracingView: "Volume Tracings",
+  ArbitraryLayout: "Arbitray View Mode",
+  OrthoLayout: "Othogonal Tracings",


oRthogonal (same in line 110)

Can you rename "Tracings" to "Mode" in these labels?

philippotto · 2018-10-08T12:54:53Z

Nice, nice, nice :) I made another comment ("Tracing" vs "Mode") and I also have another request: Can we remove the bullet points in front of the custom layout names? I think it would look better without the bullets.

MichaelBuessemeyer · 2018-10-09T08:20:06Z

@philippotto could you please have a look at this PR again? I applied the feedback, but the frontend test didn't run correctly the first time on CircleCI. But without changing a thing the second try worked out. I don't know why 🤔

philippotto · 2018-10-09T09:31:19Z

@philippotto could you please have a look at this PR again? I applied the feedback, but the frontend test did run correctly the first time on CircleCI. But without changing a thing the second try worked out. I don't know why thinking

So, both runs worked out? :)

The PR looks really good. I also just tested the conversion between the persisted layouts and it seems to work pretty well.

What's a bit weird is that I can sometimes delete the last layout (so that the list is empty) and sometimes I can't delete it. But I think that's not a blocker. Feel free to merge!

philippotto · 2018-10-09T11:26:57Z

Please wait with merging until tomorrow, as the lab is conducting live tests at the moment.

MichaelBuessemeyer · 2018-10-10T08:09:17Z

but the frontend test did run correctly the first time on CircleCI.

meant: "but the frontend test didn't run correctly the first time on CircleCI." 🙂

MichaelBuessemeyer added 13 commits September 27, 2018 17:01

added UI support, moving layout into store

f8f5f53

made reset layout items in action bar into an own component; added ac…

024c4e9

…tive layout as state to action bar; reworked layout persistence to match new layout schema

moved layout into less file, moved persisting methods

3c8bdf8

changed layout, goldenlayout adapter no gets layout via a method, and…

17fda10

… active layout is resetable

added resetting, deleting, adding a new layout; added modal to enter …

16ed9c8

…new layout name

added modal

0370fe4

fixed import errors and added saving in local storage aber deleting a…

6571528

…nd adding

corrected adding, deleting and resetting the layout

7101a39

fixed linter error

db153c5

fixed flow errors

59d9dcf

Merge branch 'master' of github.com:scalableminds/webknossos into mul…

75baf8a

…itple-tracing-custom-layouts

using lodash to create deepcopy of stored layouts

34fadc2

v1.10.1

6e19cc6

MichaelBuessemeyer self-assigned this Oct 2, 2018

MichaelBuessemeyer added enhancement frontend labels Oct 2, 2018

MichaelBuessemeyer and others added 5 commits October 2, 2018 12:23

Merge branch 'master' into mulitple-tracing-custom-layouts

fe90c6a

modified yarn.lock -> added checksum?

61b5365

Merge branch 'mulitple-tracing-custom-layouts' of github.com:scalable…

9ad7120

…minds/webknossos into mulitple-tracing-custom-layouts

removed unused comment; removed useless emit

e4371ae

Update CHANGELOG.md

c2cd24d

MichaelBuessemeyer requested a review from philippotto October 2, 2018 11:09

Update CHANGELOG.md

55c7875

philippotto changed the title ~~Mulitple custom layouts for tacing views~~ Multiple custom layouts for tacing views Oct 2, 2018

philippotto requested changes Oct 2, 2018

View reviewed changes

MichaelBuessemeyer added 4 commits October 4, 2018 12:09

Merge branch 'master' of github.com:scalableminds/webknossos into mul…

0d88758

…itple-tracing-custom-layouts

applied requested feedback

2837206

removed lastActive from filtering layout names

796d697

fixed linting error

55a903c

changed ui to indicate that layout are stored separately

1fcb25b

philippotto reviewed Oct 5, 2018

View reviewed changes

MichaelBuessemeyer added 4 commits October 5, 2018 13:32

fix default layout bug

41b4875

some visual tweaks

1a9e1cd

made the code pretty

8d0429f

fixed typos and change toast to thrown error message

45ab3f2

daniel-wer changed the title ~~Multiple custom layouts for tacing views~~ Multiple custom layouts for tracing views Oct 8, 2018

MichaelBuessemeyer added 4 commits October 8, 2018 15:16

renamed labels for layout sub menu

215bffd

fixed visual bug with the icons

74fb288

merged master into mulitple-tracing-custom-layouts

1c6289a

removed bullet points from layout list

db8d6fe

philippotto approved these changes Oct 9, 2018

View reviewed changes

Merge branch 'master' into mulitple-tracing-custom-layouts

fe5bb10

MichaelBuessemeyer merged commit 3c762e9 into master Oct 10, 2018

philippotto deleted the mulitple-tracing-custom-layouts branch January 24, 2019 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple custom layouts for tracing views #3299

Multiple custom layouts for tracing views #3299

MichaelBuessemeyer commented Oct 2, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 2, 2018

philippotto left a comment

philippotto Oct 2, 2018

philippotto Oct 2, 2018

MichaelBuessemeyer Oct 4, 2018

philippotto Oct 5, 2018

philippotto Oct 2, 2018

philippotto Oct 2, 2018

philippotto Oct 2, 2018

MichaelBuessemeyer Oct 4, 2018

philippotto Oct 5, 2018

philippotto Oct 5, 2018

philippotto Oct 8, 2018

philippotto commented Oct 8, 2018

MichaelBuessemeyer commented Oct 9, 2018 •

edited

Loading

philippotto commented Oct 9, 2018

philippotto commented Oct 9, 2018

MichaelBuessemeyer commented Oct 10, 2018

Multiple custom layouts for tracing views #3299

Multiple custom layouts for tracing views #3299

Conversation

MichaelBuessemeyer commented Oct 2, 2018 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer commented Oct 2, 2018

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto commented Oct 8, 2018

MichaelBuessemeyer commented Oct 9, 2018 • edited Loading

philippotto commented Oct 9, 2018

philippotto commented Oct 9, 2018

MichaelBuessemeyer commented Oct 10, 2018

MichaelBuessemeyer commented Oct 2, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 9, 2018 •

edited

Loading