Initial implementation of Xi-Mac status bar by nangtrongvuon · Pull Request #183 · xi-editor/xi-mac

nangtrongvuon · 2018-05-28T15:29:16Z

This PR adds a initial status bar implementation, which supports the addition, update and deletion of status items.

#180
Part of #176
To be used with xi-editor's #677

googlebot · 2018-05-28T15:29:20Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-05-28T15:29:22Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-05-28T15:31:45Z

CLAs look good, thanks!

googlebot · 2018-05-28T15:31:46Z

CLAs look good, thanks!

cmyr · 2018-05-28T22:42:30Z

Haven't had a chance to take a look at this yet, but it might be a good idea to open a PR in xi-editor with the changes to the API that support this work, and maybe also some changes to the sample plugin to use this api? Or a link to some plugin that does?

nangtrongvuon · 2018-05-28T23:07:49Z

@cmyr Will do shortly.

cmyr

Just some preliminary feedback.

cmyr · 2018-05-29T03:42:00Z

    /// A notification containing an alert message to be shown the user.
    func alert(text: String);

+    // Status bar notifications


I would follow the documentation style of the rest of this file; three /// (for doc comments) and a description of the specific command.

Sign in to view

@@ -0,0 +1,140 @@
+//


cmyr · 2018-05-29T03:43:47Z

+
+class StatusItem: NSTextField {
+
+    var key: String = ""


perhaps these should be let bindings? In which case you don't need to assign them initial values, if you assign them values in init.

cmyr · 2018-05-29T03:45:08Z

+
+class StatusBar: NSView {
+
+    private let backgroundColor = NSColor(deviceWhite: 0.9, alpha: 1.0)


this can probably change based on theme or other settings (you mentioned this, just noting it again)

cmyr · 2018-05-29T03:47:42Z

+class StatusBar: NSView {
+
+    private let backgroundColor = NSColor(deviceWhite: 0.9, alpha: 1.0)
+    private let statusBarHeight: CGFloat = 20


why 20? (just curious)

No particular reason, I just thought it looked nice.

cmyr · 2018-05-29T03:49:40Z

+    // Adds a status bar item. Only appends status bar items for now.
+    func addStatusItem(_ item: StatusItem) {
+        // If item exists, update its value
+        if let existingItem = currentItems[item.key] {


I think trying to add an item that already exists either replaces the exiting item or is an error. Having it just silently update instead seems like an easy way to let people write bugs.

Sign in to view

+    }
+
+    // Update a status bar item with a new value.
+    func updateStatusItem(_ key: String, _ value: String) {


cmyr · 2018-05-29T03:57:46Z

+        super.draw(dirtyRect)
+        let path = NSBezierPath()
+        backgroundColor.setFill()
+        __NSRectFill(dirtyRect)


I... don't think we're supposed to use this function. I think there's just a fill() method on NSRect now?

cmyr

This is getting close, but there's still some logic errors with the layout:

new items are not hidden, if they should be:

in some situations items being 'unhidden' are aligned incorrectly:

When we have the layout logic sorted we should talk about how we actually want things to look, and work on polish.

cmyr · 2018-06-03T16:03:43Z

    }

+    func setupStatusBar() {
+        if self.unifiedTitlebar == true {


we shouldn't need to do styling here. The statusbar should draw itself with the 'default style' (to be determined) by default, and if we're using the unified style we should be updating the statusbar in the same place where we update other elements (which is the unifiedTitlebar's didSet method).

To see the advantage of this approach, notice that if you change the unified_toolbar setting, the statusbar isn't updated.

Similarly, if you select a dark theme the statusbar becomes unreadable.

cmyr · 2018-06-03T16:06:36Z

        willScroll(to: scrollView.contentView.bounds.origin)
        editView.gutterCache = nil
        shadowView.updateShadowColor(newColor: theme.shadow)
+        if self.unifiedTitlebar == true {


see above; any properties of the statusbar related to this setting should be set in along with other changes related to this setting

cmyr · 2018-06-03T16:08:42Z

+
+    var backgroundColor: NSColor = NSColor.white
+    var itemTextColor: NSColor = NSColor.black
+    var statusBarPadding: CGFloat = 10


This can maybe be a let binding?

cmyr · 2018-06-03T16:11:37Z

+    var hiddenItems = [StatusItem]()
+    var leftItems = [StatusItem]()
+    var rightItems = [StatusItem]()
+    var currentItems: [StatusItem] {


instead of storing current keys and current items separately, can we just have a dictionary of keys: items?

googlebot · 2018-06-05T01:51:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-06-05T01:51:21Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-06-05T04:08:35Z

CLAs look good, thanks!

googlebot · 2018-06-05T04:08:35Z

CLAs look good, thanks!

cmyr · 2018-06-08T15:52:56Z

Getting there! Still some design/layout nits:

buggy stuff:

Three things in this example:

the 'f' item is initially shown even though it's clipped by the window
adding the 'g' item causes the 'f' to be correctly hidden
when the z item is added, the 'e' should probably be hidden; they're very close to one another.

Left padding: the left padding of the status bar item should be equal to the left padding of the buttons in the top menu bar.

typing this sequence causes the 'z' item to be inserted overlapping with the previous item:

but resizing the view even slightly correctly hides the previous item:

.. so there's some bug here where we aren't correctly hiding items on one side after adding them on another.

general design stuff:

When drawing not-unified, I think there should be a width-0 light gray line separating the statusbar from the main view. For comparison, here are three versions: without line, with line, and gradient with line:

And with the unified toolbar:

I think that the decision is less clear with the unified toolbar, but I still maybe prefer the line?

As for font size, I generally like my fonts small, but 10pt might be a bit straining on the eyes in the long run. I currently have iTerm set to 11pt so if the rest of the statusbar can be scaled to that size I'd probably go for that. Unless you want to improve readability (especially on non-retina screens), in which case 12pt is the way to go.

nangtrongvuon · 2018-06-10T05:15:14Z

I’ll iron out the remaining bugs. I do agree with the designs with the light gray line and the solid gray as I think the metallic gray should only be reserved for the title bar. It would also help when Mojave hits and people use the dark theme, as the system color used there will get updated as well.

cmyr · 2018-06-10T16:24:10Z

Awesome, look forward to playing around. I might not have time today, but tomorrow at the latest. 👌 Also there's a merge conflict that will have to get resolved at some point. :)

cmyr

Just a few little remaining observations, but this looks and feels much better. I suspect we will run across some issues as we start to use the feature, but this feels good for now.

I do have one remaining request, which I think should be a follow-up PR: I think that we should hide the statusbar unless it is showing any items; this lets us keep the default window appearance as minimal as possible.

cmyr · 2018-06-11T17:03:13Z

        currentItems[item.key] = item
-        checkItemsFitFor(windowWidth: self.superview!.frame.width)
        self.needsUpdateConstraints = true
+        checkItemsFitFor(windowWidth: self.superview!.frame.width)


shouldn't self.bounds.width always suffice? our width should be the same as the superview's.

cmyr · 2018-06-11T17:06:21Z

        return currentItems.values.filter { $0.isHidden == false }
            .map({$0.bounds.width})
-            .reduce(CGFloat(currentItems.count - 1) * statusBarPadding, +)
+            .reduce(CGFloat(currentItems.count - hiddenItems.count - 1) * statusBarPadding, +)


I think this should have braces? Subtraction isn't associative, so (currentItems.count - hiddenItems.count) - 1 != (currentItems.count - (hiddenItems.count - 1)

(I'm sure the expected behaviour is defined in the swift spec, but braces make it easier for the code reviewer)

cmyr · 2018-06-11T17:17:50Z

+
+        NSLayoutConstraint.activate([
+            statusBar.heightAnchor.constraint(equalToConstant: statusBar.statusBarHeight),
+            statusBar.widthAnchor.constraint(equalTo: editView.widthAnchor),


just noticed this, but the width constraint shouldn't be necessary if you have a leading and trailing.

… status item, refactored names for consistency

…status bar updates, addressed PR comments

…is resized

…tus bar

…e changed

…a constraints being created, adjusted padding to match buttons

…ome comments, fixed small bugs when updating constraints

cmyr · 2018-06-13T01:24:59Z

                window.backgroundColor = unifiedTitlebar ? color : nil
-
+
+                statusBar.updateStatusBarColor(newBackgroundColor: self.theme.background, newTextColor: self.theme.foreground, newUnifiedTitlebar: unifiedTitlebar)


Doing one last read through and test, I realize there's a race here: we can receive the theme_changed RPC before our viewDidAppear: is called, which means self.statusBar is nil, and this crashes.

I think there's a fairly simple solution, which is to instantiate the statusbar at init time; i.e. instead of var statusBar: StatusBar! we can just have let statusBar = StatusBar(frame: .zero).

cmyr · 2018-06-13T01:48:37Z

@lukakerr If you're interested, you might have an opinion on this?

cmyr

Okay, I'm happy with this, look forward to starting to play around with it in some plugins.

cmyr · 2018-06-24T13:06:57Z

Talked to raph yesterday and he has a bunch on his plate, so we decided it's better to get this in now than to wait for time for a second review.

As follow-up, though, I would like to have at least an option for the statusbar to not be shown if it doesn't contain any items. @nangtrongvuon could you open an issue for this?

nangtrongvuon changed the title ~~Initial implementation of Xi-Mac statusbar~~ Initial implementation of Xi-Mac status bar May 28, 2018

nangtrongvuon force-pushed the statusbar branch from 56476a9 to 23be901 Compare May 28, 2018 15:31

nangtrongvuon mentioned this pull request May 28, 2018

Added status bar rpcs to send to front end xi-editor/xi-editor#677

Merged

cmyr reviewed May 29, 2018

View reviewed changes

nangtrongvuon force-pushed the statusbar branch 2 times, most recently from 5edc78b to 06cfa4d Compare May 31, 2018 14:19

cmyr reviewed Jun 3, 2018

View reviewed changes

nangtrongvuon force-pushed the statusbar branch from 1b3a3ee to 6dcd5c4 Compare June 4, 2018 12:45

nangtrongvuon force-pushed the statusbar branch 3 times, most recently from a1388fd to 08a3232 Compare June 5, 2018 04:08

nangtrongvuon force-pushed the statusbar branch from 5ccd408 to 942ad45 Compare June 8, 2018 12:50

nangtrongvuon force-pushed the statusbar branch 2 times, most recently from abe86c7 to 98485da Compare June 10, 2018 13:52

cmyr reviewed Jun 11, 2018

View reviewed changes

Initialized work on status bar

c2e5752

nangtrongvuon added 21 commits June 12, 2018 21:27

Added initial implementation of updating constraints after removing a…

a0415aa

… status item, refactored names for consistency

Added dictionary to manage existing status item keys

daf28a3

Update constraints on status item addition

aa32659

Fixed update constraints, remove placeholders from past tests

f85834d

Added copyright notice, themed status bar to current theme, adjusted …

9a772d3

…status bar updates, addressed PR comments

Made status items sort alphabetically

6e996c3

Added initial implementation of hiding items dynamically when window …

f254331

…is resized

Fixed hiding on resize, check for unified titlebar before theming sta…

1da6795

…tus bar

Refactored code for better clarity

aaca13d

Refactored status bar again to use dictionary

476fea3

Set statusbar to only change when unified titlebar config is modified

92e3192

Implemented initial logic fix for hiding elements that overlap in sta…

7a98eff

…tus bar

Fixed case where item should still be updated when hidden

3611baa

Added hiding when items are expanded due to their values changing

853c800

Removed superflous calls to remove and add subviews whenever items ar…

3d025aa

…e changed

Refactored and refined hide behavior

a6f2c5c

Added initial attempt to replicate NSWindow color

5d50284

Removed extra checks for hidden items when updating and removing items

3d0607a

Changing unified titlebar now updates status bar color properly

9c6bc3a

Adjusted hiding behavior, added a gray line as per design, fixed extr…

a5aadf3

…a constraints being created, adjusted padding to match buttons

Refined hiding behavior to make use of all available space, rewrote s…

fa8cd8d

…ome comments, fixed small bugs when updating constraints

nangtrongvuon force-pushed the statusbar branch from 73899cd to fa8cd8d Compare June 12, 2018 14:30

Addressed PR comments

132f74f

cmyr reviewed Jun 13, 2018

View reviewed changes

Init statusbar at runtime to prevent race crash

5de63cf

cmyr approved these changes Jun 13, 2018

View reviewed changes

cmyr mentioned this pull request Jun 21, 2018

Preliminary cursor info status item support xi-editor/xi-editor#718

Closed

cmyr merged commit 80e6723 into xi-editor:master Jun 24, 2018


		class StatusBar: NSView {

		private let backgroundColor = NSColor(deviceWhite: 0.9, alpha: 1.0)

		window.backgroundColor = unifiedTitlebar ? color : nil


		statusBar.updateStatusBarColor(newBackgroundColor: self.theme.background, newTextColor: self.theme.foreground, newUnifiedTitlebar: unifiedTitlebar)

Conversation

nangtrongvuon commented May 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented May 28, 2018

Uh oh!

googlebot commented May 28, 2018

Uh oh!

googlebot commented May 28, 2018

Uh oh!

googlebot commented May 28, 2018

Uh oh!

cmyr commented May 28, 2018

Uh oh!

nangtrongvuon commented May 28, 2018

Uh oh!

cmyr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

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!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyr left a comment

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!

googlebot commented Jun 5, 2018

Uh oh!

googlebot commented Jun 5, 2018

Uh oh!

googlebot commented Jun 5, 2018

Uh oh!

googlebot commented Jun 5, 2018

Uh oh!

cmyr commented Jun 8, 2018

buggy stuff:

general design stuff:

font size:

Uh oh!

jansol commented Jun 8, 2018

Uh oh!

nangtrongvuon commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmyr commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmyr left a comment

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!

nangtrongvuon commented May 28, 2018 •

edited

Loading

nangtrongvuon commented Jun 10, 2018 •

edited

Loading

cmyr commented Jun 10, 2018 •

edited

Loading