tig commented 1 month ago

Via @tznind "we have lost support for the FrameView because drag/move doesn't work properly with it." here

https://github.com/gui-cs/TerminalGuiDesigner/pull/189

May be same issue: "Borders are disabled for now I think too for same reason (they break click drag)"

tig commented 1 month ago

@tznind With this:

https://github.com/gui-cs/Terminal.Gui/pull/3634

ANY View is now draggable:

Set Arrangement = ViewArrangement.Movable | ViewArrangement.Overlapped

(Obviously if you want tiled, don't set the Overlapped flag)

By default FrameView in v2 is NOT enabled for either of these.

But Window is (and when #2491 is done, it will NOT be derived from Toplevel and will be basically the same as FrameView with a shadow and different border).

tig commented 1 month ago

WIth #2537 any view will be sizable too.

tig commented 1 month ago

My plan with #2537 is to leverage IDesignable such that if design mode is enabled, Border will be able to be "hidden" such that a View without a Border can be dragged.

Same for dragging multiple views.

BDisp commented 1 month ago

Since the TGD as access to internal TG objects may be could have some kind of mechanism that allow TGD to enable drag and restore defaults when finish drag.

tznind commented 1 month ago

There is a LOT of code in TGD to do this drag. Complex bits include:

Preventing drag for axis which are not compatible Pos type (e.g. lock X to Pos.Right of Something but still drag along Y axis)
Undo/Redo
Drag to sub/super view
Multi view drag
Keyboard support

It's probably a good 10% of the designer codebase. And it's heavily coupled to other bits of code like

view selection system
Operation stack
Global mouse manager.

Not saying we can't make it part of core, just that it's going to get complex fast 😀 but up for trying.

Regarding internal, I think policy is to try and avoid.

tig commented 1 month ago

Since the TGD as access to internal TG objects may be could have some kind of mechanism that allow TGD to enable drag and restore defaults when finish drag.

As previously discussed, we want to move away from having TGD have internal access. We should make these mechanisms public.

Of course, we CAN do this over time, but I'd like to really try to make it happen before V2 release!

tig commented 1 month ago

There is a LOT of code in TGD to do this drag. Complex bits include:

Preventing drag for axis which are not compatible Pos type (e.g. lock X to Pos.Right of Something but still drag along Y axis)

Undo/Redo

Drag to sub/super view

Multi view drag

Keyboard support

It's probably a good 10% of the designer codebase. And it's heavily coupled to other bits of code like

view selection system

Operation stack

Global mouse manager.

Not saying we can't make it part of core, just that it's going to get complex fast 😀 but up for trying.

Regarding internal, I think policy is to try and avoid.

How willing are you to migrate this code into TG as needed?

tznind commented 1 month ago

I can try but it is going to be difficult. And not fast because of scale.

Designer basically has a complete parallel mouse handling system based on global handler and hit detection.

tig commented 1 month ago

I can try but it is going to be difficult. And not fast because of scale. Designer basically has a complete parallel mouse handling system based on global handler and hit detection.

My head's still not clear on this, but I'm now thinking the place to rally here is around IDesignable.

We put it on View and expand it with whatever methods/properties TGD needs. Since I've moved most mouse handling code into Application and out of View & Toplevel (more do do), we can put the right logic in Application.Mouse.cs to enable calling out to TGD (and other designers). We can even go as far as adding a 4th Adornment that is only lit up when IDesignable.DesignMode == true that sits between Border and Padding that TGD can leverage.

The goal would be to enable TGD to minimize throwing away as much as that code as possible whilst leveraging the new architecture in TG V2.

Just spitballing of course to get the juices flowing. This will be an interesting challenge. I'll repeat again: We don't ship V2 until TGD is amazing on it.

BDisp commented 1 month ago

Instead of adding a new Adornment why not enabling all the mouse features anywhere on a view if IDesignable.DesignMode == true. So, the mouse will have the same features on Margin, Border, Paddingand Viewport.

tznind commented 1 month ago

I think we should split to 2 goals

Enable TGD to function like it did before with Borders/FrameView.
Migrate TGD resize/move code to core

The first will enable a polished use experience and full range of Views supported in TGD in relatively short period of time.

The second is far more ambitious and will take longer.

1. Enable FrameView and Borders as simply as possible

I will have to refresh my memory on the exact issue with Borders and FrameView but I think it was around hit detection and the 'out of the box' FrameView being moveable. Probably can fix that in CreateSubControlDesign where it explicitly makes everything focusable we can just make everything immoveable and let the original TGD move code take over.

There are already some Border related hacks that TGD applies in its extension version of FindDeepestView e.g. see UnpackHitView

/// <summary>
/// <para>
/// Sometimes <see cref="View.FindDeepestView"/> returns what the library considers
/// the clicked View rather than what the user would expect.  For example clicking in
/// the <see cref="Border"/> area of a <see cref="View"/>.
/// </para>
/// <para>This works out what the real view is.</para>
/// </summary>
/// <param name="hit"></param>
/// <returns></returns>
public static View? UnpackHitView(this View? hit)
{

      if (hit != null && hit.GetType().Name.Equals("TabRowView"))
      {
          hit = hit.SuperView;
      }

      // Translate clicks in the border as the real View being clicked
      if (hit is Border b)
      {
          hit = b.Parent;

      }

      // TabView nesting of 'fake' views goes:
      // TabView
      //   - TabViewRow
      //   - View (pane)
      //     - Border (note you need Parent not SuperView to find Border parent)

      if (hit?.SuperView is TabView tv)
      {
          hit = tv;
      }

      return hit;
  }

2. Migrate 'design' code from TGD to Terminal.Gui

This is the Design class which is at the core of TGD.

It is how TGD distinguishes between Views the user created e.g. a TabView and those that are subcomponents that are not ineded to be treated seperately e.g. the TextField in a ColorPicker or the TabRowView in a TabView.

We might start tackling this feature with a new namespace Terminal.Gui.Designer rather than trying to put everything under an IDesignable interface which is on all views?

tig commented 1 month ago

Re: UnpackHitView and FindDeepestView...

Adornments can have Subviews, and before v2 ships there will be at least several places where this happens:

Border:

3407
2814

Padding:

2489

Note that Adornments is not finished yet, and my thinking is finishing the issues above will lead to fixing most of what's not finished. Some tracking issues:

2994
3406

This is relevant because FindDeepestView NEEDS to (and does) traverse into subviews of a Border, which, in-turn may have Borders with subviews...

I don't think TGD needs to support the design of such things, but it DOES need to be aware.

Instead of TGD having a special UnpackHitView, I'd prefer to figure out how FindDeepestView can do what an app like TGD needs: Find the deepest View, ignoring Adornments.

Right?

tig commented 1 month ago

2. Migrate 'design' code from TGD to Terminal.Gui

This is the Design class which is at the core of TGD.

It is how TGD distinguishes between Views the user created e.g. a TabView and those that are subcomponents that are not ineded to be treated seperately e.g. the TextField in a ColorPicker or the TabRowView in a TabView.

Wow, I see a lot of issues in https://github.com/gui-cs/TerminalGuiDesigner/blob/v2/src/Design.cs that could be addressed with fixes in TG.

Text - should be an attribute of a View somehow, not a hard-coded list in TGD:

    /// <summary>
    /// View Types for which <see cref="View.Text"/> does not make sense as a user
    /// configurable field (e.g. there is a Title field instead).
    /// </summary>
    private readonly HashSet<Type> excludeTextPropertyFor = new()
    {

ALL Views have Title. If Border.Thickness.Top > 0 the Title will be displayed. AND I want it to work with Border.Thickness.Left > 0 too with a vertical-oriented Title.

It is true that not all views need a Text property. Back in v1, we moved Text into View and there were good reasons for that. But in v2, perhaps we back this out somewhat? For example, we could define ISupportsText with a default impl, similar to how I recently did IOrientation.

Would this allow you to simplify Design.cs?

Defn of `Container` is weird

First, this terminology is confusing

    /// <summary>
    /// Gets a value indicating whether <see cref="View"/> is a 'container' (i.e. designed
    /// to hold other sub-controls.
    /// </summary>

container -> SuperView control -> View

Second, if a View is public whether it uses Subviews or direct code should be completely encapsulated and TGD should not need to know that. By knowing (or guessing) it means the View in question can't change it's implementation without TGD changing.

Can we figure out a better way for TGD to deal with this?

For example, what if we extended the currently private View.InternalSubViews to not mean "Internal to TG" but to be "Internal to this". IOW, if a View does this.Add(subview), TGD should feel free to navigate into it. But if a View does this.InternalSubViews.Add(subview) TGD would ignore it?

Or, maybe just a new property on View: public DesignVisibility DesignVisibility { get; set; } where DesignVisibility is an enum with Visible, HideFromDesigners. TGD would ignore any view where view.DesignVisibility.HasFlag(HideFromDesigners) is true. Then TabView would set DesignVisibilty to HideFromDesigners on TabRow before calling Add?

`DesignableProperties` should be discoverable

It seems cray to me that TGD has so much hard-coded code for what properties on a View are interesting for design or not.

It seems we could easily come up with a way for TGD to inspect a View for, say, an [DesignableProperty] attribute.

We might start tackling this feature with a new namespace Terminal.Gui.Designer rather than trying to put everything under an IDesignable interface which is on all views?

I now concur that just IDesignable is not enough. Not sure a new namespace is the solution either. I think we could tackle each of the three things I point out above relatively independently.

tig commented 1 month ago

https://github.com/gui-cs/TerminalGuiDesigner/blob/86c83f8d6e2a920a6c6e6899efca470d058169b9/src/Design.cs#L170C1-L173C32

FWIW, I plan on addressing the cray that led to this in

3627

tig commented 1 month ago

https://github.com/gui-cs/TerminalGuiDesigner/blob/86c83f8d6e2a920a6c6e6899efca470d058169b9/src/Design.cs#L150-L158

I view any View that uses a ContentView as a hack that should be re-factored. The need for ContentView derives from issues in View that have been addressed in v2.

The only remaining examples of ContentView in v2 are:

TileView / Tile - Will be deprecated once #2483 is completed.
ScrollView - Will be deprecated via #2489
TabView - I've looked enough to know it can be re-factored to not have a ContentView fairly easily.
Wizard - Will be addressed in #2485

tig commented 1 month ago

https://github.com/gui-cs/TerminalGuiDesigner/blob/86c83f8d6e2a920a6c6e6899efca470d058169b9/src/Design.cs#L319C1-L328C6

Is there any reason this concept of IOperation can't be merged with the TG concept of Command?

If we were able to do that, there'd be no need for TGD to have carnal/internal knowledge of the various views in GetExtraOperations().

tig commented 1 month ago

From #3490 which I'm now closing as a dupe of this:

We should find all places in TGD where reflection is used to get to the internals of TG and open an Issue for each to address.

PosExtensions
DimExtensions
ColorSchemes - TGD.FindDeclaredColorScheme
ViewExtensions

etc...

tznind commented 1 month ago

Instead of TGD having a special UnpackHitView, I'd prefer to figure out how FindDeepestView can do what an app like TGD needs: Find the deepest View, ignoring Adornments.

I am not sure ignoring Adornments is exactly what is required. I think TGD can be smarter about it. Lets look at the use case.

When you click somewhere TGD wants to work out what the most 'user meaningful' thing(s) there are in the click hierarchy. It does this by inspecting the hit and all the SuperViews:

So when we right click the View in a TabView we will show:

operations relevant to the TabView
but also the actual View right clicked (i.e. AddView).

Looking at the results of UnpackHitView we see we are 4 deep:

The only issue I think with Adornments is that they have Parent instead of SuperView. Probably TGD can just do something like

var actualSuperView = v is IAdornment ? v.Parent : v.SuperView

tznind commented 1 month ago

For example, we could define ISupportsText with a default impl, similar to how I recently did IOrientation.

Would this allow you to simplify Design.cs?

That sounds sensible, so that check for making Text designable would just be v is ISupportsText.

The only thing to be careful about is inheritance where a subview decides it doesn't want Text to be supported but not a big deal.

tznind commented 1 month ago

Defn of Container is weird [...] if a View is public whether it uses Subviews or direct code should be completely encapsulated and TGD should not need to know that.

Being a 'container' determines whether the user is able to drag and drop other views into it.

This covers the use case:

User adds a View (the actual class not a subclass) onto the design area
User then drags views into it
User then adds Border and Title to the View

If user drags a View over a TextField then instead of the dragged view becomming a subview of TextField it would just hover over it (i.e. siblings).

So we need to know by class what is designed to have stuff dropped into it at design time. But having an attribute DesignedToHoldSubviews would probably would do the trick (probably needs a better name).

view.DesignVisibility.HasFlag(HideFromDesigners)

I don't think this is relevant to whether a view is a container.

tznind commented 1 month ago

It seems cray to me that TGD has so much hard-coded code for what properties on a View are interesting for design or not.

It seems we could easily come up with a way for TGD to inspect a View for, say, an [DesignableProperty] attribute.

I agree, it will simplify code and also potentially allow design of user custom Views later in a consistent manner.

One thing to be careful of is that TGD needs to robustly support the property that means:

TGD knows how to deal with the Type of the property
TGD needs to know if the Property is 'special'
- Should it be designable but have a hard coded 'design time' value e.g. Visible (these use SuppressedProperty class instead of Property
- The property doesn't do anything weird when changed at runtime e.g. throw exceptions

We also need to decorate 'sub properties' not just those on the root View itself e.g. those accessed via Style or other paths.

TGD can then use reflection to step into the sub properties and identify those that can be designed.

Background

The current approach of explicitly listing in TGD the properties comes from stability point of view.

It is important that TGD does not offer properties that then crash when you try to modify them or save/load them (code gen/compilation).

For this reason I was very conservative about adding properties and only enabling them when there was adequate unit testing and manual testing for each property

gui-cs / Terminal.Gui

TGD: Master Issue tracking TG issues that must be fixed in v2 #3650

1. Enable FrameView and Borders as simply as possible

2. Migrate 'design' code from TGD to Terminal.Gui

3407

2814

2489

2994

3406

2. Migrate 'design' code from TGD to Terminal.Gui

Text - should be an attribute of a View somehow, not a hard-coded list in TGD:

Defn of `Container` is weird

`DesignableProperties` should be discoverable

3627

Background

gui-cs / Terminal.Gui

TGD: Master Issue tracking TG issues that must be fixed in v2 #3650

1. Enable FrameView and Borders as simply as possible

2. Migrate 'design' code from TGD to Terminal.Gui

3407

2814

2489

2994

3406

2. Migrate 'design' code from TGD to Terminal.Gui

Text - should be an attribute of a View somehow, not a hard-coded list in TGD:

Defn of Container is weird

DesignableProperties should be discoverable

3627

Background

Defn of `Container` is weird

`DesignableProperties` should be discoverable