Add settings to store initial scene position

[gautamv95]

Fixes #876

[4ian]

Seems like a good start :) I've added some comments about a few things, let me know if that make sense

[gautamv95]

@4ian is this a breaking change?
Since the games created with the old version of libGD.js won't have viewPositionValid, viewXPosition, viewYPosition in their JSON files.
Should I add a check for that as well?

[4ian]

Since the games created with the old version of libGD.js won't have viewPositionValid, viewXPosition, viewYPosition in their JSON files.
Should I add a check for that as well?

No need for it, because the default values will be false for viewPositionValid.

You handled properly the update of the options when a scrollbar is moved, but you should also save the options when the view position is updated in onViewPositionChanged, which means the view was moved for another reason than the scrollbars (for example, pinch on touch screens, or space + drag'n'drop) :)

[gautamv95]

No need for it, because the default values will be false for viewPositionValid.

Okay great :)

You handled properly the update of the options when a scrollbar is moved, but you should also save the options when the view position is updated in onViewPositionChanged, which means the view was moved for another reason than the scrollbars (for example, pinch on touch screens, or space + drag'n'drop) :)

Will take a look

[gautamv95]

My initial commit didn't actually work on touch devices, forgot some changes, fixed them now

[4ian]

I think this function is not properly named, and doing too much.

Think of how the class is used:
The parent, InstancesEditor, must create PinchHandler by passing it a viewPosition. This means that InstancesEditor knows, and actually owns viewPosition.
Now it must also pass it something called "setViewPosition" used to... set a new ViewPosition? But why it would need to do that, because InstancesEditor is actually the owner of ViewPosition.

You probably did this because you observed that onViewPositionChanged is passing the ViewPosition as argument. This is right... but onViewPositionChanged is a prop of InstancesEditor! This means that it needs to pass ViewPosition because the parent of InstancesEditor does not have access to ViewPosition - because it's owned by InstancesEditor.

Here, it does not really make sense to pass to PinchHandler a function that takes a ViewPosition. Think like the parent (InstancesEditor): "why are you passing my a ViewPosition? I know ViewPosition, it's mine! It's me that passed it to you, why are you giving it back in your callback? I don't even need it, leave me in peace!" ;)
Or think like the child (PinchHandler): "why are you asking me for the ViewPosition? It's you that passed it to me!".

Instead, what you want is to get notified when the view is moved. And PinchHandler is moving the position, by calling scrollBy.
The solution:

• pass a function called onViewPositionChanged to PinchHandler, which is called (as the name implies) "when the view position is changed". But without argument: _onViewPositionChanged: () => void .
  Then it's the responsibility of the parent to react accordingly, providing more info if needed to it's parent. In other words, in InstancesEditor, you do:

  onViewPositionChanged: () => { this.props.onViewPositionChanged(this.viewPosition) }
  

Actually there is maybe a better solution. Why is InstancesEditor having to call manually onViewPositionChanged, and now why is PinchHandler having to call its own onViewPositionChanged every time we call scrollBy (and scrollToInstance)?

It's manual and error-prone, we could forget it in the future... instead why not give this responsibility to... ViewPosition.

    this.viewPosition = new ViewPosition({
      initialViewX: project ? project.getGameResolutionWidth() / 2 : 0,
      initialViewY: project ? project.getGameResolutionHeight() / 2 : 0,
      width: this.props.width,
      height: this.props.height,
      options: this.props.options,
      onViewPositionChanged: () => { this.props.onViewPositionChanged(this.viewPosition); }
    });

and then you can call onViewPositionChanged inside scrollBy and scrollToInstance.
And you remove all the other calls to this.props.onViewPositionChanged - they are now redundant because ViewPosition is doing the job of notifying after any change.

Hope this make sense. :)

[arthuro555]

@gautamv95 do you plan on finishing this PR?

[Silver-Streak]

Checking in on old PRs and issues for some cleanup.

I think this is already completed. When I open a scene now, it shows up in whatever position I last hit save on. Do we know if this is still needed @arthuro555 @4ian ?

Or are we good to close this and #876 out?

[arthuro555]

I'm pretty sure this can be closed, yes.