search

gnustep / libs-gui

The GNUstep gui library is a library of graphical user interface classes written completely in the Objective-C language; the classes are based upon Apple's Cocoa framework (which came from the OpenStep specification). *** Larger patches require copyright assignment to FSF. please file bugs here. ***
http://www.gnustep.org
GNU General Public License v3.0
279 stars 103 forks source link

Add new 10.6 delegate methods to NSBrowserDelegate #281

Closed gcasa closed 3 months ago

gcasa commented 4 months ago

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

gcasa commented 4 months ago

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

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

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

gcasa commented 4 months ago

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 commented 4 months ago

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.

gcasa commented 4 months ago

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 commented 4 months ago

@fredkiefer Please review when possible. GC

gcasa commented 4 months ago

Multiple selection works just as on macOS: multiple_selection_browser_gs For comparison: multiple_selection_macos

gcasa commented 4 months ago

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 commented 4 months ago

Also... 1) what is the complex case here? 2) 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 commented 4 months ago

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

gcasa commented 4 months ago

@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 commented 3 months ago

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

gcasa commented 3 months ago

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

gcasa commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.