Reduce the free space between viewports in tracing #3333

MichaelBuessemeyer · 2018-10-08T12:48:11Z

This PR adjusts the default width of a layout container of a viewport to its height so that there will be no more unused whitespace.

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Open a tracing an reset the layout. There should be no more whitespace between the viewports.
close all tracings, clear the local storage and then open a tracing. The default layout should not have any free space between the viewports.
There is a maximum width for the viewports, so that the right menu is always visible
Test this by resizing the window to a dimension where the height is greater than the width and the reset the layout. The right menu should be still visible.

Issues:

fixes Reduce gaps in default layout by using window dimensions in definition #3298

~~[ ] Updated changelog~~
~~[ ] Updated migration guide if applicable~~
~~[ ] Updated documentation if applicable~~
~~[ ] Needs datastore update after deployment~~
Ready for review

…usted to current window aspact ratio

MichaelBuessemeyer · 2018-10-09T10:38:35Z

@philippotto I removed all changes and added the requested viewport width adjustment to the default layout. So this PR should be ready for a review.

Edit
Do you think a changelog entry is necessary? I don't think so, because these changes do not have a direct impact to the user.

philippotto

Nice! Works well and the code also looks good (except for the 500; see my other comment).

One thing though: There is still a noticable gap on my screen. Could it be that the height of the tab title has to be used during the calculation?

philippotto · 2018-10-10T12:51:26Z

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

+const unmemoizedGetDefaultLayouts = () => {
+  let { height, width } = getGroundTruthLayoutRect();
+  // prevent default height and width
+  if (height === 500 && width === 500 && window.innerWidth) {


Can we avoid these magic numbers? Two possible solutions:

getGroundTruthLayoutRect either returns the proper height/width or null (but never the dummy values of 500). then, other callers of getGroundTruthLayoutRect would need to check for null and then use the 500 fallback value if this makes sense

or: use window.innerWidth etc. inside getGroundTruthLayoutRect as a fallback. However, we have to be careful here, since the layout is a bit fickle during initialization. if the rect is too large, it can happen that the layout is initialized too big and that scrollbars screw up the correct dimensions. this is why, I'd go with the first option.

…ling on

…tracing_layout_view

MichaelBuessemeyer · 2018-10-14T14:42:15Z

@philippotto
I applied the feedback of you but while doing so I ran into two kinda strange bug/errors:
The first is an error by flow, that appears when trying to export the headerHeight from tracing_layout_view. The actual trigger for this error is importing the constant to default_layout_configs.
The error thrown, says that the children of NmlUploadZoneContainer are not of the same type. 1.
2..
For a quickfix I move the constant into the Constants.js

The second error appeared with the new way of handling the default-layouts: Importing default_layout_configs.js into the store increases the "load/compile-time" => the time until the actual store object is available. But other modules like the test modules do not wait for this. This creates a lot of testing errors. Removing the defaultLayoutSchema from the default state of the store and also removing the whole import solves the problem. So I just set the layouts to an empty object.
The Problem: The same error appears in layout_persistence when directly trying to load the stored layouts with: Store.dispatch(setStoredLayoutsAction(readStoredLayoutConfigs()));. Here I added a timeout as a quick workaround.

In both cases I don't think my solution is very good. So can we talk about this next time we see each other at office 🙂 ?

…eaderHeight back to tracing_layout_view

MichaelBuessemeyer · 2018-10-18T05:05:10Z

@philippotto I resolved the circular imports and also adjusted the width evaluation to the height of the viewport layouts (removed the header height from the evaluation) => This PR should be ready for another review.

philippotto · 2018-10-22T07:12:06Z

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

+const unmemoizedGetDefaultLayouts = () => {
+  const layoutContainer = getGroundTruthLayoutRect();
+  layoutContainer.height -= layoutHeaderHeight * 2;
+  const viewportWidth = Math.min(((layoutContainer.height / 2) * 100) / layoutContainer.width, 40);


Where is the 40 coming from?

The idea behind it, is that i want the viewport to have at least a width of 40px. I put the 40 into a constant and changed Math.minto Math.max.
Or do you @philippotto think that a minimum width is not needed?

Ah wait, having a close look at the case, where the minimum would apply tells me that this idea is nonsense 🙂

philippotto · 2018-10-22T07:13:31Z

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

+const unmemoizedGetDefaultLayouts = () => {
+  const layoutContainer = getGroundTruthLayoutRect();
+  layoutContainer.height -= layoutHeaderHeight * 2;
+  const viewportWidth = Math.min(((layoutContainer.height / 2) * 100) / layoutContainer.width, 40);


Where is the 40 coming from?

philippotto · 2018-10-22T07:18:55Z

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

+type ExtractReturn<Fn> = $Call<<T>(() => T) => T, Fn>;
+type Layout = $Keys<ExtractReturn<typeof unmemoizedGetDefaultLayouts>>;
+
+export const getDefaultLayoutSchema = () => {


I'm not sure Schema is the best word choice here. How about Config?
Also, I think it would be better if the function name also reflected that it resets the default layouts.

What do you think of getCurrentDefaultLayoutConfig?

philippotto · 2018-10-22T07:19:45Z

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

-      const { width } = getGroundTruthLayoutRect();
+      let { width } = getGroundTruthLayoutRect();
+      if (width === undefined) {
+        width = 500;


Now, the 500 is scattered around in this file. Can you extract it into a dummyExtent variable?

philippotto · 2018-10-22T07:39:29Z

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

-      const { width } = getGroundTruthLayoutRect();
+      let { width } = getGroundTruthLayoutRect();
+      if (width === undefined) {
+        width = 500;


Now, the 500 is scattered around in this file. Can you extract it into a dummyExtent variable?

philippotto · 2018-10-23T16:40:32Z

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

-      const { width } = getGroundTruthLayoutRect();
+      let { width } = getGroundTruthLayoutRect();
+      if (width === undefined) {
+        width = 500;


Now, the 500 is scattered around in this file. Can you extract it into a minimumExtent variable?

philippotto · 2018-10-23T16:42:11Z

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

      }
      // migrate to newset schema
      const withMulipleLayoutsSchema = {
        OrthoLayoutView: {
-          "Custom Layout": layouts.OrthoLayoutView || defaultLayouts.OrthoLayoutView,
+          "Custom Layout": layouts.OrthoLayoutView || getDefaultLayouts().OrthoLayoutView,


Please store getDefaultLayouts() in a variable (above line 40) and use that within the four occurrences here (I know, that the method call is memoized, but this is not clear to the reader of this function and also this might change in the future).

philippotto · 2018-10-23T16:45:25Z

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

@@ -66,7 +62,11 @@ const updateSizeForGl = gl => {
  if (!container) {
    return;
  }
-  const { width, height } = getGroundTruthLayoutRect();
+  let { width, height } = getGroundTruthLayoutRect();


I'd create a function "castOptionalExtent" which takes width and height and turns them into 500 if they are undefined. So that you can write: const { width, height } = castOptionalExtent(getGroundTruthLayoutRect());. This pattern can be used at the multiple places, too.

The current version of getGroundTruthLayoutRectguarantees to return a valid width and height (that is 500 when every way of determining the bounds failed). So there is no need to handle this anymore. I think I had already removed the checks like in line 52, but I think I was wrong ...

MichaelBuessemeyer · 2018-10-25T06:04:23Z

@philippotto Could you please have a look at the changes I commited and the comments I wrote. I also found another circular import that is causing errors. The key here is to not import tracing_layout_view in default_layout_configs. So I moved headerHeight into default_layout_configs but I do not like the idea. Do you have an suggestions where to put it?

Maybe add a new constant to Constants.js that has all needed constants for the layouting stored?

philippotto · 2018-10-25T15:21:57Z

I just tested your PR here (https://adddefaultwidescreenlayout.webknossos.xyz/datasets/Connectomics_Department/2017-05-31_mSEM_scMS109_bk_100um_v01/view#41472,18944,1536,0,1.30) and the reduction of whitespace seems to work pretty well! However, the stored layouts seem to reset for me?

Steps to reproduce:

create a new layout "test"
customize it
switch layout to default
switch back to "test" layout <-- this layout looks exactly like the default layout and misses my customizations

MichaelBuessemeyer · 2018-11-02T10:47:31Z

@philippotto fixed it 😄 🎉

philippotto

Nice 👍 This will be a valuable improvement for the users.

* master: Fix rgb support (#3455) Fix docker uid/gid + binaryData permissions. Persist postgres db (#3428) Script to merge volume tracing into on-disk segmentation (#3431) Hotfix for editing TaskTypes (#3451) fix keyboardjs module (#3450) Fix guessed dataset boundingbox for non-zero-aligned datasets (#3437) voxeliterator now checks if the passed map has elements (#3405) integrate .importjs (#3436) Re-write logic for selecting zoom level and support non-uniform buckets per dimension (#3398) fixing selecting bug and improving style of layout dropdown (#3443) refresh screenshots (#3445) Reduce the free space between viewports in tracing (#3333) Scala linter and formatter (#3357) ignore reported datasets of non-existent organization (#3438) Only provide shortcut for tree search and not for comment search (#3407) Update Datastore+Tracingstore Standalone Deployment Templates (#3424) In yarn refresh-schema, also invalidate Tables.scala (#3430) Remove BaseDirService that watched binaryData symlinks (#3416) Ensure that resolutions array is dense (#3406) Fix bucket-collection related rendering bug (#3409)

added default layouts for wide screen windows and layout reset is adj…

5fad76b

…usted to current window aspact ratio

MichaelBuessemeyer added enhancement frontend labels Oct 8, 2018

MichaelBuessemeyer self-assigned this Oct 8, 2018

MichaelBuessemeyer requested a review from philippotto October 8, 2018 12:48

undo previous changes; added viewport width ajustment for default layout

8d0618c

MichaelBuessemeyer changed the title ~~Added default layouts for wide screen windows~~ Reduce the free space between viewports in tracing Oct 10, 2018

change default layouts into a cache function

43d127f

philippotto requested changes Oct 10, 2018

View reviewed changes

MichaelBuessemeyer added 4 commits October 12, 2018 15:12

removed default return value from groundtruthrect and passed the hand…

4ed74e3

…ling on

merged master into this branch and resolved conflicts

f28771a

added untracked new files

bb9c72d

resolved merge conflicts with flow, had to move header_height out of …

45479fe

…tracing_layout_view

MichaelBuessemeyer added 4 commits October 14, 2018 17:03

fix store loading to slow problem; using own memoize function

f80a833

moved back to lodash memoize

7636c2f

resolved cicular imports errors by changing import structure; moved h…

bc8e92f

…eaderHeight back to tracing_layout_view

made code pretty

3952276

philippotto reviewed Oct 23, 2018

View reviewed changes

MichaelBuessemeyer added 2 commits October 25, 2018 07:27

applied feedback

b4b7043

fixed circular import

814ed05

MichaelBuessemeyer and others added 2 commits November 2, 2018 10:45

fix bug that always resets the layout when changing layouts

b16a31e

Merge branch 'master' into add-default-wide-screen-layout

b8c2f68

philippotto approved these changes Nov 5, 2018

View reviewed changes

fm3 added the Epic label Nov 5, 2018

Merge branch 'master' into add-default-wide-screen-layout

1f14a86

MichaelBuessemeyer merged commit 67ce04c into master Nov 8, 2018

daniel-wer deleted the add-default-wide-screen-layout branch November 9, 2018 14:46

daniel-wer mentioned this pull request Nov 9, 2018

Refresh screenshots #3445

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the free space between viewports in tracing #3333

Reduce the free space between viewports in tracing #3333

MichaelBuessemeyer commented Oct 8, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 9, 2018 •

edited

Loading

philippotto left a comment

philippotto Oct 10, 2018

MichaelBuessemeyer commented Oct 14, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 18, 2018

philippotto Oct 22, 2018

MichaelBuessemeyer Oct 25, 2018

MichaelBuessemeyer Oct 25, 2018

philippotto Oct 22, 2018

philippotto Oct 22, 2018

MichaelBuessemeyer Oct 25, 2018 •

edited

Loading

philippotto Oct 22, 2018

philippotto Oct 22, 2018

philippotto Oct 23, 2018

philippotto Oct 23, 2018

philippotto Oct 23, 2018

MichaelBuessemeyer Oct 25, 2018

MichaelBuessemeyer commented Oct 25, 2018

philippotto commented Oct 25, 2018

MichaelBuessemeyer commented Nov 2, 2018

philippotto left a comment

Reduce the free space between viewports in tracing #3333

Reduce the free space between viewports in tracing #3333

Conversation

MichaelBuessemeyer commented Oct 8, 2018 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer commented Oct 9, 2018 • edited Loading

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Oct 14, 2018 • edited Loading

MichaelBuessemeyer commented Oct 18, 2018

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

MichaelBuessemeyer Oct 25, 2018 • edited Loading

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

MichaelBuessemeyer commented Oct 25, 2018

philippotto commented Oct 25, 2018

MichaelBuessemeyer commented Nov 2, 2018

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Oct 8, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 9, 2018 •

edited

Loading

MichaelBuessemeyer commented Oct 14, 2018 •

edited

Loading

MichaelBuessemeyer Oct 25, 2018 •

edited

Loading