Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

Replaces current GTK Toolbox with a native one #6324

Merged
merged 82 commits into from
Nov 23, 2018
Merged

Replaces current GTK Toolbox with a native one #6324

merged 82 commits into from
Nov 23, 2018

Conversation

netonjm
Copy link
Contributor

@netonjm netonjm commented Oct 15, 2018 •
edited
Loading

testdragcomplete

**
Important note related to colors and style:
I asked @vancura about this and he told me to leave the native styles / colors of Cocoa until they meet in Boston to decide which ones normalize to use.
**

The current toolbar includes all behaviour than GTK control.

  • Elements without tools shows a message
  • Drag and drop from toolbar to text editor
  • Double click paste selected element to editor
  • Light theme/Dark theme
  • Item contextual menu
  • Tab must change between elements
  • Includes different modes of visualization: Section on/off and Only images on/off
  • Search bar
  • Add toolbox items button
  • Sections expanding

Additional considerations for testing:

  • Data must be based in document selected, no document shows no data with a message
  • Property panel should change accordingly with the selected object
  • I added configuration for DebugMac/ReleaseMac in project
  • Created a class with Helpers to use with native stuff, probably reusable by other taks
    NativeViewHelper.cs
  • Also created a internal focus chain system to tab next/previous between subviews views inside the Toolbox, it backs focus to Gtk when index ends/starts

It also includes Accessibility in all elements from the Toolbar from both modes of visualization and

Fixes Bug #637219
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/637219

TODO:

  • Enable feature with a EnvVar
  • Accessibility navigation
  • Tunneling events from Gtk to Cocoa
  • Right context menu
  • Loading screen
  • Tooltip
  • Fix Drag and Drop
  • Fix expander wrong behaviour
  • Fix wrong alignents and sizes with NSCollectionView items
  • Visual fixes (7 left)
  • Light theme

@netonjm netonjm self-assigned this Oct 15, 2018
@netonjm netonjm changed the title [WIP] Creates a native Toolbox view and enables using an environment variable Creates a native Toolbox view and enables using an environment variable Oct 25, 2018
@netonjm netonjm force-pushed the master-cocoa-toolbox branch 4 times, most recently from 9fbbabe to 30093ef Compare October 25, 2018 17:47
rodrmoya
rodrmoya reviewed Oct 26, 2018
View reviewed changes
main/src/addins/MonoDevelop.DesignerSupport/MonoDevelop.DesignerSupport.Toolbox/MacToolbox.cs
{
nativeChildViews.Add (view);
view.Focused += (s, e) => ChangeFocusedView (s as INativeChildView);

Copy link
Contributor

Choose a reason for hiding this comment

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

same here, but I guess this would be better implemented using the Action/Target cocoa pattern, so that you don't have to keep a list of all views you subscribed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what do you mean with the list of all views 🤔. This code only exectutes when a view is added to the internal chain loop. This array represents all the elements we want include in the chain to focus, because the tab navigation is broken in native.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Therzok can confirm, but IIRC the Action/Target is better for memory leaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/mono/monodevelop/wiki/UI-code---Reference-counting-and-you#xamarinmac

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great! I will try to use this in the future, thanks for the info! 👍

@netonjm
Changes to use Dictionary instead a List
4b62c79
@netonjm
Removes EnvironmentVariable check to enables by default
97d42d9
@netonjm
Handles un-sync states when control ask for number of items in databo…
a18e35f 
…und stages

When we change the current document selected we ask for the service to get new
categories and items, when this happens, there is a small time of un-sync state
when the control expects a number of items from a sections and the new one could be
minor than the older. this could throw an NRE (index of out range)
@netonjm
Includes additional checks to catch out of sync contexts
2b6aa28
@netonjm
Fixes failures in tests disposing only when it's needed
8b55b22
@netonjm
Adds CALayer rendering to improve the performance + accessibility fixes
186635e 
To improve the scrollbar framerate we had to move to use the CALayer rendering
engine, needed some changes fixes in our accessibility implementation
@netonjm
Fixes overlap with scrollers in NSScrollView when contents has CALayers
b1965fa
@slluis
Copy link
Member

slluis commented Nov 16, 2018

It work much better now. A remaining problem is that keyboard handling has problems. Cursor down moves selection correctly, but cursor up does a big jump when trying to go from one category to another.

@slluis
Copy link
Member

slluis commented Nov 19, 2018

@iainx can you review again?

@netonjm netonjm changed the title Creates a native Toolbox view and enables using an environment variable Replaces current GTK Toolbox with a native one Nov 19, 2018
iainx
iainx suggested changes Nov 20, 2018
View reviewed changes
Copy link
Contributor

@iainx iainx left a comment

Choose a reason for hiding this comment

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

A few accessibility notes:

When in full mode, you can't single click to add a text snippet to the editor, only a double click works. For accessibility I think a single click should be ok if you implement AccessibilityPerformClick

In compact mode you can't add a text snippet to the editor at all, neither single click nor double click do anything. Maybe the images that are used as the items need to implement INSAccessibilityButton

There appears to be an extra group in-between the pad and the contents that could be hidden from the accessibility without any problems.

@netonjm
Copy link
Contributor Author

netonjm commented Nov 20, 2018 •
edited
Loading

Fixed @iainx ! We added support for each item like an accessibility button and also cover keyboard (ENTER key performs the activation in the selected item) ❤️

@slluis slluis added the verified The feature or bug fix has been tested and it works label Nov 23, 2018
@slluis slluis merged commit f1ac7d3 into master Nov 23, 2018
@slluis slluis deleted the master-cocoa-toolbox branch November 23, 2018 09:27
@garuma garuma mentioned this pull request Nov 24, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rodrmoya rodrmoya rodrmoya left review comments

@iainx iainx iainx approved these changes

@slluis slluis slluis approved these changes

@Therzok Therzok Therzok approved these changes

@jtorres jtorres Awaiting requested review from jtorres

@sevoku sevoku Awaiting requested review from sevoku

Assignees

@netonjm netonjm

Labels
verified The feature or bug fix has been tested and it works
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@netonjm @rodrmoya @slluis @vancura @Therzok @iainx