Windows Separation Remastered: Preferences Window #618

lolgear · 2019-12-11T14:51:04Z

Preferences window controller has been encapsulated.
Preferences window controller has been moved to its own xib.

lolgear · 2019-12-11T14:54:21Z

@lucasderraugh Ready, I guess :) ( except layout, heh. I will add constraints a bit later. )

lucasderraugh · 2019-12-12T04:06:14Z

@lucasderraugh Ready, I guess :) ( except layout, heh. I will add constraints a bit later. )

I'll wait until you think it's all done haha. Looks pretty good so far though.

…ded.

lolgear · 2019-12-12T08:15:15Z

@lucasderraugh Ok, layout has been added.
It still contains warnings "prior OS X 10.11". Well, maybe it is time to set deployment target to something newer than 10.10? I guess that 10.11 is pretty good.

lolgear · 2019-12-14T14:11:20Z

@lucasderraugh
If we can raise deployment target, all this layout can be easily done by NSGridView.
It is available since 10.12.

lolgear · 2019-12-17T08:31:43Z

@lucasderraugh Go-go-go-go-go :)

lucasderraugh · 2019-12-17T22:09:28Z

Sorry, I merged the text size changes in first as those were changes made based on a branch many years ago. If you get the chance to look over the conflicts we can merge these in as well. I think we can consider bumping later, but for now constraints will work just fine. We already moved to 10.10 this year from 10.8 to get over some dark mode hurtles with older releases. I think perhaps we can consider moving to 10.12 on the next release of macOS.

…indow

lolgear · 2019-12-17T22:34:45Z

@lucasderraugh
Fixed.

lolgear · 2019-12-18T11:11:23Z

@lucasderraugh
If everything is fine and you would like to merge it, I guess it could be release candidate.
But, of course, I would like to merge other PRs also, especially #573 ❤️

lolgear · 2019-12-18T11:15:44Z

@lucasderraugh
Also #583 could enhance UX for most of us...

lucasderraugh

A few feedback points, overall seems like it should be ok.

lucasderraugh · 2019-12-28T22:59:08Z

GitUp/Application/PreferencesWindowController.m

+- (void)showWindow:(id)sender {
+  // sync NSUserDefaults
+  NSString* theme = [NSUserDefaults.standardUserDefaults stringForKey:kUserDefaultsKey_Theme];
+  self.selectedTheme = theme;
+  NSString* channel = [NSUserDefaults.standardUserDefaults stringForKey:kUserDefaultsKey_ReleaseChannel];
+  self.selectedChannel = channel;
+  [self selectPreferencePane:nil];
+  [super showWindow:sender];
+}


I don't think we need to resync these every time we open the window. They shouldn't be changing other than via the preferences pane. Also, every time you trigger the show preferences window, it is moving it down. Looks like the window frame setup used to be done in awakeFromNib?

@lucasderraugh
Heh, weird.

[self selectPreferencePane:self]; // non-nil sender

fixed it.

Maybe we can remove checks for sender here:

- (IBAction)selectPreferencePane:(id)sender { NSWindow *window = self.window; [_preferencesTabView selectTabViewItemWithIdentifier:_preferencesToolbar.selectedItemIdentifier]; NSSize size = NSSizeFromString(_preferencesTabView.selectedTabViewItem.label); NSRect rect = [window contentRectForFrameRect:window.frame]; // if (sender) { rect.origin.y += rect.size.height; // } rect.size.width = size.width; rect.size.height = size.height; // if (sender) { rect.origin.y -= rect.size.height; // } [window setFrame:[window frameRectForContentRect:rect] display:YES animate:(sender ? YES : NO)]; }

I'm not sure really in what cases there is or isn't a sender? I would assume there always is one. I just want the preference pane to never move itself down haha.

@lucasderraugh Fixed.

lucasderraugh · 2019-12-28T23:06:02Z

GitUpKit/Utilities/GILaunchServicesLocator.h

+extern NSString* const GIViewController_DiffTool;
+extern NSString* const GIViewController_MergeTool;
+extern NSString* const GIViewController_TerminalTool;


Why GIViewController?

@lucasderraugh Legacy, I guess.
GIPreferences_ prefix is better?

// Change extern NSString *const GIPreferences_DiffTool; extern NSString *const GIPreferences_MergeTool; extern NSString *const GIPreferences_TerminalTool; // DiffTool and MergeTool extern NSString *const GIPreferences_DiffMergeTool_FileMerge; extern NSString *const GIPreferences_DiffMergeTool_Kaleidoscope; extern NSString *const GIPreferences_DiffMergeTool_BeyondCompare; extern NSString *const GIPreferences_DiffMergeTool_P4Merge; extern NSString *const GIPreferences_DiffMergeTool_GitTool; extern NSString *const GIPreferences_DiffMergeTool_DiffMerge; // TerminalTool extern NSString *const GIPreferences_TerminalTool_Terminal; extern NSString *const GIPreferences_TerminalTool_iTerm;

Also I would like to add suffix to distinguish Keys and DisplayNames.

extern NSString *const GIPreferences_DiffTool_Key; extern NSString *const GIPreferences_DiffMergeTool_FileMerge_DisplayName;

Ah, I see. I think GIPreferences works well. I think you can shorten up some of these names. If it's just being used as the localized string value, just GIPreferences_FileMerge would work fine if the string is just going to be "File Merge" anyways.

@lucasderraugh Fixed.

lucasderraugh · 2019-12-28T23:14:37Z

GitUpKit/Utilities/GILaunchServicesLocator.m

+ (NSString *)bundleIdentifierForDisplayName:(NSString *)displayName {
+  if ([displayName isEqualToString:GIViewController_TerminalTool_iTerm]) {
+    return GIViewController_TerminalTool_iTerm_BundleIdentifier;
+  }
+  return nil;
+}
+ (NSString *)standardDefaultsKeyForDisplayName:(NSString *)displayName {
+  if ([displayName isEqualToString:GIViewController_TerminalTool_iTerm]) {
+    return GIViewController_TerminalTool_iTerm_Key;
+  }
+  return nil;
+}


Static analysis warning for returning nil when expecting non-nil return.

@lucasderraugh Fixed.

lucasderraugh

I always forget to hit the request changes button...

lucasderraugh · 2019-12-30T18:08:36Z

GitUp/Application/PreferencesWindowController.h

+@property (nonatomic, copy) NSArray <NSString *>*channelTitles;
+@property (nonatomic, copy) NSArray <NSString *>*themesTitles;


Nit: Generics right next to type: NSArray< (no space)

@property (nonatomic, copy) NSArray<NSString *>*themesTitles; // or @property (nonatomic, copy) NSArray<NSString*>*themesTitles; // or @property (nonatomic, copy) NSArray<NSString*> *themesTitles;

The style throughout the rest of the project is to have the type and then the identifier after a space so my vote would be NSArray<NSString*>* propertyName. Not how my personal projects would go, but it's not really my repo 😅.

@lucasderraugh Fixed. :3

… have been fixed.

…en fixed.

lolgear · 2020-01-08T14:07:31Z

@lucasderraugh Go-go-go

…indow

lolgear · 2020-01-16T14:52:06Z

Ready?

* kit: launch services locator has been added. * application: windows preferences window controller has been added. * application: project has been updated. * application: app delegate has been cleaned up. * application: windows xibs main menu preferences window has been removed. * application: common constants have been cleaned up. * application: project has been updated. * application: windows xibs preferences window constraints have been added. * application: preference text size has been fixed. * kit: launch services locator preferences keys have been renamed. Bugs have been fixed. * application: document has been fixed. * application: windows xibs preferences window layout has been updated. * application: windows preferences window controller small bugs have been fixed. * Fix bad merge from master Co-authored-by: Lucas Derraugh <lucas@derraugh.com>

lolgear added 7 commits December 11, 2019 17:37

kit: launch services locator has been added.

14f1114

application: windows preferences window controller has been added.

fe1dff0

application: project has been updated.

06655e3

application: app delegate has been cleaned up.

4df9509

application: windows xibs main menu preferences window has been removed.

b1af4f7

application: common constants have been cleaned up.

00568c1

application: project has been updated.

9029e77

application: windows xibs preferences window constraints have been ad…

8b3697c

…ded.

Merge upstream/master into window_separation_remastered_preferences_w…

712e40a

…indow

application: preference text size has been fixed.

60c5d51

lucasderraugh approved these changes Dec 28, 2019

View reviewed changes

lucasderraugh requested changes Dec 30, 2019

View reviewed changes

lucasderraugh reviewed Dec 30, 2019

View reviewed changes

lolgear requested a review from lucasderraugh January 4, 2020 14:51

lolgear added 4 commits January 7, 2020 11:47

kit: launch services locator preferences keys have been renamed. Bugs…

060433c

… have been fixed.

application: document has been fixed.

02a6b27

application: windows xibs preferences window layout has been updated.

866292f

application: windows preferences window controller small bugs have be…

617e7c5

…en fixed.

lucasderraugh added 2 commits January 12, 2020 10:16

Merge branch 'master' into window_separation_remastered_preferences_w…

d0240fc

…indow

Fix bad merge from master

681d36f

lucasderraugh merged commit ec81a92 into git-up:master Jan 20, 2020

lolgear deleted the window_separation_remastered_preferences_window branch May 17, 2021 09:33

lolgear restored the window_separation_remastered_preferences_window branch May 18, 2021 08:58

		@property (nonatomic, copy) NSArray <NSString >channelTitles;
		@property (nonatomic, copy) NSArray <NSString >themesTitles;

Windows Separation Remastered: Preferences Window #618

Windows Separation Remastered: Preferences Window #618

Uh oh!

Conversation

lolgear commented Dec 11, 2019

Uh oh!

lolgear commented Dec 11, 2019

Uh oh!

lucasderraugh commented Dec 12, 2019

Uh oh!

lolgear commented Dec 12, 2019

Uh oh!

lolgear commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolgear commented Dec 17, 2019

Uh oh!

lucasderraugh commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lolgear commented Dec 17, 2019

Uh oh!

lolgear commented Dec 18, 2019

Uh oh!

lolgear commented Dec 18, 2019

Uh oh!

lucasderraugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolgear Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasderraugh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasderraugh Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolgear commented Jan 8, 2020

Uh oh!

lolgear commented Jan 16, 2020

Uh oh!

Uh oh!

lolgear commented Dec 14, 2019 •

edited

Loading

lucasderraugh commented Dec 17, 2019 •

edited

Loading

lolgear Jan 4, 2020 •

edited

Loading

lucasderraugh left a comment •

edited

Loading

lucasderraugh Jan 5, 2020 •

edited

Loading