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

Fixes #2482. Refactor Redraw - Non-virtual with the right set of virtual OnXXX methods. #2577

Merged
merged 15 commits into from
May 4, 2023

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Apr 27, 2023

Fixes #2482 - Renaming Redraw to OnDraw and creating a method Draw with the entire draw process phases.

Draw call:

OnDrawFrames
OnDrawContent -> cancelable.
  OnDraw
OnRenderLineCanvas
OnDrawContentComplete

Others can also be cancelable.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested review from migueldeicaza and tig as code owners April 27, 2023 02:49
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

Nice work!

Please see my comments. Let me know if I wasn't clear.

Terminal.Gui/Application.cs Show resolved Hide resolved
Terminal.Gui/View/ViewDrawing.cs Outdated Show resolved Hide resolved
Terminal.Gui/View/ViewDrawing.cs Show resolved Hide resolved
Terminal.Gui/View/ViewDrawing.cs Outdated Show resolved Hide resolved
Terminal.Gui/View/ViewEventArgs.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/Button.cs Show resolved Hide resolved
@BDisp
Copy link
Collaborator Author

BDisp commented Apr 28, 2023

@tig, @tznind please correct me if I'm wrong. If I want to prioritize an event to called before a overridden method is run, then is a danger invoking the event inside the virtual method. To avoid this the DrawContent must be invoked before and if it's canceled then the OnDrawContent won't be called . e.g. In this case bellow the DrawContent event will never be invoked. I think this is bad, right? The user must have a chance to use this event and return what he want.

			if (!OnDrawContent (Bounds)) {
				OnDraw ();
			}
		public virtual bool OnDrawContent (Rect contentArea)
		{
			var dev = new DrawEventArgs (contentArea);
			DrawContent?.Invoke (this, dev);

			return dev.Cancel;
		}
		public override bool OnDrawContent (Rect contentArea)
		{
			return true;
		}

I think this should be:

			var dev = new DrawEventArgs (contentArea);
			DrawContent?.Invoke (this, dev);
			if (!dev.Cancel) {
				OnDrawContent (Bounds);
			}

This way the DrawContent event is invoked and if it's canceled then the OnDrawContent method will not be called. If not canceled the OnDrawContent will be called. The OnDraw method isn't needed really.

@tig
Copy link
Collaborator

tig commented Apr 28, 2023

The OnDraw method isn't needed really.

I think you are spot on.

@tig
Copy link
Collaborator

tig commented May 1, 2023

@BDisp see merge conflicts.

@BDisp BDisp marked this pull request as draft May 1, 2023 08:47
tznind and others added 10 commits May 2, 2023 17:43
….DataTable (gui-cs#2576)

* WIP: Add ITableDataSource

* WIP: Refactor TableView

* WIP: Port CSVEditor

* WIP: Port TableEditor

* WIP: Port MultiColouredTable scenario

* Fix bug of adding duplicate column styles

* Update tests to use DataTableSource

* Tidy up

* Add EnumerableTableDataSource<T>

* Add test for EnumerableTableDataSource

* Add test for EnumerableTableDataSource

* Add code example to xmldoc

* Add ProcessTable scenario

* Rename ITableDataSource to ITableSource and update docs

* Rename EnumerableTableDataSource to EnumerableTableSource

* Fixed Frame != Bounds; changed UICat Scenarios list to use tableview!

* Fix scroll resetting in ProcessTable scenario

* Fix unit tests by setting Frame to same as Bounds

* Document why we have to measure our data for use with TableView

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
…cs#2583)

* WIP: Add ITableDataSource

* WIP: Refactor TableView

* WIP: Port CSVEditor

* WIP: Port TableEditor

* WIP: Port MultiColouredTable scenario

* Fix bug of adding duplicate column styles

* Update tests to use DataTableSource

* Tidy up

* Add EnumerableTableDataSource<T>

* Add test for EnumerableTableDataSource

* Add test for EnumerableTableDataSource

* Add code example to xmldoc

* Add ProcessTable scenario

* Rename ITableDataSource to ITableSource and update docs

* Rename EnumerableTableDataSource to EnumerableTableSource

* Fixed Frame != Bounds; changed UICat Scenarios list to use tableview!

* Fix scroll resetting in ProcessTable scenario

* Fix unit tests by setting Frame to same as Bounds

* Document why we have to measure our data for use with TableView

* WIP: Simplify FileDialogs use of TableView

* WIP start migrating sorter

* WIP new filedialog table source mostly working

* WIP remove sorter class

* Refactor GetOrderByValue to be adjacent to GetColumnValue

* Fix collection navigator back so it ignores icon

* Fix unit tests

* Tidy up

* Fix UseColors

* Add test for UseColors

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
… unlike TextField (gui-cs#2572)

* Fixes gui-cs#2196. TextView: Setting Text places cursor at beginning, unlike TextField

* Change all private members to have the _prefix.

* Renamed local member to prevLayoutStyle.

* Helper function for SetNeedsDisplay.
…#2570)

* Fixes gui-cs#2569. Borders scenarios needed to be refactored.

* Fix border title with width equal to 4 and thickness top grater than 1.

* Improves border manipulation on borders scenarios.

* Prevents null value on the margin, border and padding thickness on the border scenarios.

* Remove NStack using dependence and fix prior commit.

* Refactoring the Frames scenario.

* Deleted borders scenarios.

* I did not realize that it was changed to SetSubViewNeedsDisplay.

* Re-layout; fixed colorpicker; fixed radio group

* Remove Thickness.IsEmpty as requested.

* Change the Frames scenario as requested.

---------

Co-authored-by: Tig Kindel <tig@users.noreply.github.com>
…-cs#2584)

* Builds collectionnav support into UI cat for TableView

* Fixes keyboard mapping

* MultiSelect = false for TableView

* MultiSelect = false doesn't unbind ctrl-a
…ew (gui-cs#2586)

* Refactor CollectionNavigator to a base and a collection implementation

* Refactor CollectionNavigatorBase to look for first match smartly

* Add TableCollectionNavigator

* Make TableCollectionNavigator a core part of TableView

* Fix bad merge

* Added tests for tableview collection navigator

* Add FileDialogCollectionNavigator which ignores . and directory separator prefixes on file names

* whitespace fixes

---------

Co-authored-by: Tig <tig@users.noreply.github.com>
@BDisp BDisp marked this pull request as ready for review May 2, 2023 19:06
@BDisp
Copy link
Collaborator Author

BDisp commented May 3, 2023

Due the ScreenToView fix the mouse grabbing on the Clipping scenario doesn't jumping anymore. The ScreenToBounds was removed because it doesn't used and thus is unnecessary.

screen-to-view-fix

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

Really great work!

@tig tig merged commit ea24de3 into gui-cs:v2_develop May 4, 2023
@BDisp BDisp deleted the v2_refactor-redraw_2482 branch May 4, 2023 11:18
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.

3 participants