Add new 10.6 delegate methods to NSBrowserDelegate #281

gcasa · 2024-07-19T16:06:20Z

The purpose of this branch is to add the NSBrowserDelegate methods defined in the documentation for macOS 10.6+ here...

https://developer.apple.com/documentation/appkit/nsbrowserdelegate/1407572-browser?language=objc

The test for this branch is here...

https://github.com/gcasa/NSBrowser_test

…performLoadOfColumn

gcasa · 2024-07-25T12:35:33Z

Here is the browser with an item-based delegate on mac...

Here is the browser with an item-based delegate on gnustep...

And, of course, here is Gorm to demonstrate that the previous implementation still works...

gcasa · 2024-07-25T12:36:28Z

I am going to do some cleanup, also there are some trivial changes needed to support loading an NSViewController if one is necessary for the title view, etc.

gcasa · 2024-07-25T14:05:25Z

I believe that support for the following delegate methods should be left for a separate PR:

- (BOOL) browser: (NSBrowser *)browser
  shouldEditItem: (id)item;
- (id) browser: (NSBrowser *)browser
       setObjectValue: (id)object
       forItem: (id)item;
- (NSViewController *) browser: (NSBrowser *)browser
                       previewViewControllerForLeafItem: (id)item;
- (NSViewController *) browser: (NSBrowser *)browser
                       headerViewControllerForItem: (id)item;
- (void) browser: (NSBrowser *)browser
         didChangeLastColumn: (NSInteger)oldLastColumn
        toColumn: (NSInteger)column;

As they might require significant changes to how NSBrowser is implemented. I am researching this now to see if it's possible in the current implementation.

…d, call method to get a view controller from the delegate

gcasa · 2024-07-26T15:13:11Z

The code to call the delegate method

- (NSViewController *) browser: (NSBrowser *)browser
                       headerViewControllerForItem: (id)item;

has been implemented. The rest will have to wait for a separate PR as the ones involved in editing will require a more substantial change. The other two are not strictly necessary as GS doesn't currently support column re-arrangement in NSBrowser and the leaf preview is optional.

gcasa · 2024-07-26T15:13:36Z

@fredkiefer Please review when possible. GC

fredkiefer

I think that this code will work in the most common case. I am unsure whether it is overall the correct solution. It might well be that I am missing some concept here.

Headers/AppKit/NSBrowser.h

Source/NSBrowser.m

fredkiefer · 2024-07-27T16:07:01Z

Source/NSBrowser.m

+	  if (matrix != nil)
+	    {
+	      NSArray *selectedCells = [matrix selectedCells];
+	      if (selectedCells != nil && [selectedCells count] > 0)


The whole logic of this method seems strange to me. What is your intention here?
This will work for some very common cases, for example when a new column needs to be loaded because we double click on an item in the column before. But I am not sure that every time we need to load a new column there is always exactly one item selected in the previous column.

The idea is to find the item that needs to be loaded for a given column. When multiple items are selected it will show nothing on macOS. I will make changes to make this code behave similarly. See below this screenshot from macOS:

The implementation does not match the description you give above. For multiple selections the first gets used.

Source/NSBrowser.m

gcasa · 2024-07-27T20:56:15Z

Multiple selection works just as on macOS:

For comparison:

…edkiefer

…edkiefer

gcasa · 2024-07-31T20:07:21Z

I think that this code will work in the most common case. I am unsure whether it is overall the correct solution. It might well be that I am missing some concept here.

I am actually unsure how it's not the correct solution.

gcasa · 2024-07-31T21:47:24Z

Also...

what is the complex case here?
even if it does only handle the "simple case" it is infinitely better to be able to do that than to handle NONE of them that use this type of delegate.

gcasa · 2024-08-04T02:29:53Z

I am going to enhance the test to see if I can address any of the other possible use-cases.

gcasa · 2024-08-04T02:46:36Z

@fredkiefer I have added extra data to the test. I believe that the changes I have made should cover most, if not all cases. Please take a look.

gcasa · 2024-08-08T23:01:48Z

@fredkiefer Are there any additional changes you feel are necessary here?

gcasa · 2024-08-18T18:42:40Z

@fredkiefer The behavior here matches that on macOS. Do you have any final review comments?

fredkiefer

Just fix these minor issues and then the code should be ready for a merge.

Headers/AppKit/NSBrowser.h

fredkiefer · 2024-08-18T20:05:32Z

Source/NSBrowser.m

+	  if (matrix != nil)
+	    {
+	      NSArray *selectedCells = [matrix selectedCells];
+	      if (selectedCells != nil && [selectedCells count] > 0)


The implementation does not match the description you give above. For multiple selections the first gets used.

gcasa · 2024-08-18T21:56:58Z

As you can see from the above it does seem to function properly. The _itemForColumn: method gets the list for the current column based on the previous column's selection. So given this, I believe it is okay to merge. I am going to do that.

fredkiefer · 2024-08-19T06:28:20Z

As you can see from the above it does seem to function properly. The _itemForColumn: method gets the list for the current column based on the previous column's selection. So given this, I believe it is okay to merge. I am going to do that.

In the discussion before this one, which you marked as resolved you wrote:

The idea is to find the item that needs to be loaded for a given column. When multiple items are selected it will show nothing on macOS. I will make changes to make this code behave similarly.

I don't see in the code how the case of multiple selection gets handled in this way. This is a minor issue, so merging was fine, still it would have been nice to get a real answer to this question.

gcasa · 2024-08-19T12:19:03Z

As you can see from the above it does seem to function properly. The _itemForColumn: method gets the list for the current column based on the previous column's selection. So given this, I believe it is okay to merge. I am going to do that.

In the discussion before this one, which you marked as resolved you wrote:

The idea is to find the item that needs to be loaded for a given column. When multiple items are selected it will show nothing on macOS. I will make changes to make this code behave similarly.

I don't see in the code how the case of multiple selection gets handled in this way. This is a minor issue, so merging was fine, still it would have been nice to get a real answer to this question.

I apologize. If you run the test and select two things in a column the column next to it shows nothing. This is the correct behavior. I will take a look and provide a better explanation of this. I am not in front of my computer right now.

gcasa · 2024-08-21T00:10:44Z

Source/NSBrowser.m

   -browser:selectRow:inColumn: [NSMatrix-selectCellAtRow:column:]</p>
 */
- (void) selectRow: (NSInteger)row inColumn: (NSInteger)column 
+- (void) selectRow: (NSInteger)row inColumn: (NSInteger)column


@fredkiefer I believe this is the method where a multiple selection causes the last column to be unloaded. The method you commented on earlier is responsible for getting the selection of the previous column so that the current column knows what to display (based on the children it retrieves).

gcasa added 7 commits July 19, 2024 12:04

Add new 10.6 delegate methods to NSBrowserDelegate

b1d0ecc

Initial changes to start loading each column based on items

1df45b7

Add delegate calls to method _performLoadOfItem:forColumn:

a1dd317

Partially correct implementation

ffdf8fb

Add code to reset column if the delegate is item-based

f5decac

Consolidate _performLoad methods, get the last item selected

782321e

Get the previous column when loading a column in item-based mode in _…

ab737b8

…performLoadOfColumn

gcasa marked this pull request as ready for review July 25, 2024 12:23

gcasa requested a review from fredkiefer as a code owner July 25, 2024 12:23

Minor whitespace cleanup

3ae1a4b

gcasa added 2 commits July 25, 2024 10:18

Update changelog with NSBrowser change details

b77c705

Move logic for finding the item for a given column to a private metho…

7130461

…d, call method to get a view controller from the delegate

gcasa requested a review from rfm July 26, 2024 22:40

Minor cleanup

9712490

fredkiefer reviewed Jul 27, 2024

View reviewed changes

gcasa added 2 commits July 27, 2024 13:49

Add back informational comments

1d266d7

Fix indentation, per discussion with @fredkiefer

ddc4765

gcasa added 2 commits July 27, 2024 16:59

Update to simplify code per discussion with @fredkiefer

4e95021

Check for methods for an item-based delegate, per discussion with @fr…

7b78d9b

…edkiefer

Merge branch 'master' into NSBrowser_branch

c85e4fc

fredkiefer approved these changes Aug 18, 2024

View reviewed changes

Final fixes per discussion with @fredkiefer

4813800

gcasa merged commit 97ea7c5 into master Aug 19, 2024
4 checks passed

gcasa deleted the NSBrowser_branch branch August 19, 2024 01:08

gcasa commented Aug 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new 10.6 delegate methods to NSBrowserDelegate #281

Add new 10.6 delegate methods to NSBrowserDelegate #281

gcasa commented Jul 19, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 26, 2024

gcasa commented Jul 26, 2024

fredkiefer left a comment

fredkiefer Jul 27, 2024

gcasa Jul 27, 2024

fredkiefer Aug 18, 2024

gcasa commented Jul 27, 2024 •

edited

Loading

gcasa commented Jul 31, 2024

gcasa commented Jul 31, 2024

gcasa commented Aug 4, 2024

gcasa commented Aug 4, 2024

gcasa commented Aug 8, 2024

gcasa commented Aug 18, 2024

fredkiefer left a comment

fredkiefer Aug 18, 2024

gcasa commented Aug 18, 2024 •

edited

Loading

fredkiefer commented Aug 19, 2024

gcasa commented Aug 19, 2024

gcasa Aug 21, 2024

Add new 10.6 delegate methods to NSBrowserDelegate #281

Add new 10.6 delegate methods to NSBrowserDelegate #281

Conversation

gcasa commented Jul 19, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 25, 2024

gcasa commented Jul 26, 2024

gcasa commented Jul 26, 2024

fredkiefer left a comment

Choose a reason for hiding this comment

fredkiefer Jul 27, 2024

Choose a reason for hiding this comment

gcasa Jul 27, 2024

Choose a reason for hiding this comment

fredkiefer Aug 18, 2024

Choose a reason for hiding this comment

gcasa commented Jul 27, 2024 • edited Loading

gcasa commented Jul 31, 2024

gcasa commented Jul 31, 2024

gcasa commented Aug 4, 2024

gcasa commented Aug 4, 2024

gcasa commented Aug 8, 2024

gcasa commented Aug 18, 2024

fredkiefer left a comment

Choose a reason for hiding this comment

fredkiefer Aug 18, 2024

Choose a reason for hiding this comment

gcasa commented Aug 18, 2024 • edited Loading

fredkiefer commented Aug 19, 2024

gcasa commented Aug 19, 2024

gcasa Aug 21, 2024

Choose a reason for hiding this comment

gcasa commented Jul 27, 2024 •

edited

Loading

gcasa commented Aug 18, 2024 •

edited

Loading