Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom layout #832

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

Add custom layout #832

wants to merge 18 commits into from

Conversation

SilvanVerhoeven
Copy link
Contributor

@SilvanVerhoeven SilvanVerhoeven commented May 27, 2021

Closes #626

Copy link
Contributor

@linusha linusha left a comment

Choose a reason for hiding this comment

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

I might investigate further tomorrow but I noticed the following:

Opening the editor for the first time in a world now leads to an Error because position (I do not remember on what, sorry) cannot be set to undefined.

@frcroth
Copy link
Contributor

frcroth commented Jun 1, 2021

I might investigate further tomorrow but I noticed the following:

Opening the editor for the first time in a world now leads to an Error because position (I do not remember on what, sorry) cannot be set to undefined.

TypeError: Cannot set property 'position' of undefined

    at InteractivesEditor_relayout_ (http://localhost:9011/qinoq/editor.js:272:45)
    at callOrSetTarget (http://localhost:9011/lively.bindings/index.js:556:82)
    at AttributeConnection_update_ (http://localhost:9011/lively.bindings/index.js:563:92)
    at eval (http://localhost:9011/lively.bindings/index.js:624:23)
    at Array.forEach (<anonymous>)
    at InteractivesEditor.eval [as extent] (http://localhost:9011/lively.bindings/index.js:622:48)
    at InteractivesEditor.Morph_setBounds_ (http://localhost:9011/lively.morphic/morph.js:904:27)
    at Window_relayoutWindowControls_ (http://localhost:9011/lively.components/window.js:252:36)
    at Window.set [as title] (http://localhost:9011/lively.components/window.js:906:26)

Which makes sense, since extent may be set before the panels are initialized

Copy link
Contributor

@frcroth frcroth left a comment

Choose a reason for hiding this comment

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

Code is ok, but I think we may need to discuss what we expect a full sized editor to look like

@SilvanVerhoeven
Copy link
Contributor Author

I might investigate further tomorrow but I noticed the following:

Opening the editor for the first time in a world now leads to an Error because position (I do not remember on what, sorry) cannot be set to undefined.

Fixed that and another bug

@SilvanVerhoeven
Copy link
Contributor Author

Code is ok, but I think we may need to discuss what we expect a full sized editor to look like

Before or after merge?

@frcroth
Copy link
Contributor

frcroth commented Jun 1, 2021

Code is ok, but I think we may need to discuss what we expect a full sized editor to look like

Before or after merge?

Before

@T4rikA
Copy link
Contributor

T4rikA commented Jun 1, 2021

Also scrolling is not possible in the complete preview, just in the left half

@frcroth
Copy link
Contributor

frcroth commented Jun 3, 2021

Can not open the editor on this branch currently
image

@frcroth frcroth marked this pull request as draft June 3, 2021 10:05
@SilvanVerhoeven SilvanVerhoeven marked this pull request as ready for review June 3, 2021 10:22
@frcroth frcroth linked an issue Jun 3, 2021 that may be closed by this pull request
Copy link
Contributor

@linusha linusha left a comment

Choose a reason for hiding this comment

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

I feel that it should not be necessary, but maybe @Paula-Kli or @SilvanVerhoeven can confirm - are deserialization checks necessary for extent on the InteractiveGraph the ResizablePanel or for _latestSubWindowRatio?

I'd also be in favor of merging this after the necessary PR in lively.next was merged, would that be ok? If you'd prefer help bringing the changes that are necessary for Robins approval on the way we can tackle it on Monday @SilvanVerhoeven!

Comment on lines +135 to +141
if (this.interactive &&
this.interactive.sequences.includes(Sequence.getSequenceOfMorph(morph))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an intended change or was it introduced by windows formatting complications?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't Sequence.getSequenceOfMorph already check whether there is a Sequence in the interactive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@linusha This is intended
@Paula-Kli No, it only traverses the morph owner change until the first Sequence

@SilvanVerhoeven
Copy link
Contributor Author

Open Problems with this PR

* [x]   Tree is too high

* [ ]  Zooming the Interactive and Resizing the top panel do not play good together

* [ ]  Saving should be investigated before merging, has not been checked until now

We won't change something about the zooming, as there are dependencies with other branches

@SilvanVerhoeven
Copy link
Contributor Author

SilvanVerhoeven commented Jun 14, 2021

grafik

@frcroth
Copy link
Contributor

frcroth commented Jun 14, 2021

Standard sequence graph extent seems off (This only happens with the first editor in a new world)
image

@@ -227,6 +272,7 @@ export class InteractivesEditor extends QinoqMorph {

connect(this.interactive, 'remove', this, 'reset');
connect(this.interactive, '_length', this.ui.menuBar.ui.scrollPositionInput, 'max').update(this.interactive.length);
// TODO: let this work with zoom
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate if this stays for now please.

extent: {
set (extent) {
this.setProperty('extent', extent);
// if (this._deserializing) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this save to remove? If so, can it go for good?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the same as the check below I would appreciate if we use this instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, can/should be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference?

editor.js Outdated

this.ui.interactiveGraph.extent = pt(
this.ui.interactiveGraph.width,
topWindowHeight - this.ui.interactiveGraph.submorphs[0].height -
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the submorphs[0] do? Aren't there two submorphs in the interactiveGraph

Copy link
Contributor

@T4rikA T4rikA left a comment

Choose a reason for hiding this comment

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

When the deserializing discussion is resolved

T4rikA and others added 2 commits June 14, 2021 13:50
Co-authored-by: T4rikA <[email protected]>
Co-authored-by: linusha <[email protected]>
Co-authored-by: frcroth <[email protected]>
Co-authored-by: Paula-Kli <[email protected]>
@linusha
Copy link
Contributor

linusha commented Jun 14, 2021

I re-requested reviews from everybody again since there were so many changes in here that I do not think that these actually reflect the review state. Feel free to just re-approve if you think otherwise. Personally, I'll not add a new review before we are done here.

Comment on lines +38 to +39
this.tree.extent = adjustedExtent;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set the extent of the tree to the same as the overall extent? Shouldn't the tree be smaller as the search field needs to be subbtracted

@linusha
Copy link
Contributor

linusha commented Jun 15, 2021

Attention

  • commit f1e3bbb will be merged separately, needs to be dropped her
  • The interactive and its aspect ratio do currently not play well with the relayouting, i.e. change whenever the window of the editor is e.g., clicked. This would need to be tackled before merging this.

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.

Responsive inspector layouting Better Scaling for the Editor
5 participants