Tail feature #486

sjoshid · 2019-12-17T17:40:23Z

Closes xi-editor/xi-editor#922.
This PR adds a "Tail File" option under Debug menu. This option can be enabled/disabled per file.

~~TODO: Gray out this option if the file doesnt exist. No point in tailing a file that doesnt exist.~~

Review Checklist

I have responded to reviews and made changes where appropriate.
I have tested the code
I have updated comments / documentation related to the changes I made.
I have rebased my PR branch onto xi-mac/master.

Removing toggleTailConfigChanged added as a dup during merge.

nangtrongvuon

The PR looks much better! I have a few remaining comments before I'm ok with merging this, though I think it's pretty close already.

nangtrongvuon · 2019-12-18T14:00:53Z

Sources/XiEditor/EditViewController.swift

@@ -711,6 +717,10 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc
        document.xiCore.setTheme(themeName: sender.title)
    }

+    @IBAction func debugToggleTail(_ sender: NSMenuItem) {
+        document.xiCore.toggleTailConfig(identifier: document.coreViewIdentifier!, enabled: !self.isTailEnabled)


We could probably rewrite this in a way so we don't have to do !self.isTailEnabled here, since it feels counter intuitive.

Rewrite how?

This is just a little nit, but I'd prefer if this was

document.xiCore.toggleTailConfig(identifier: document.coreViewIdentifier!, enabled: self.isTailEnabled)

nangtrongvuon · 2019-12-18T14:02:18Z

Sources/XiEditor/Client.swift

@@ -86,4 +86,7 @@ protocol XiClient: AnyObject {

    /// A notification containing the current replace status.
    func replaceStatus(viewIdentifier: String, status: ReplaceStatus)
+
+    /// A notification telling toggle tail config was successfully changed.
+    func toggleTailConfigChanged(viewIdentifier: String, isTailEnabled: Bool)


Why is this toggleTailConfigChanged? Do you envision the tail config to have more things than whether if the user enables tailing? Even if so, I feel like this should just be something like enableTailing.

Ok. I have made this change.

sjoshid · 2019-12-18T22:14:46Z

~~There is one small thing remaining. Gray out "Tail" option if the file doesnt exist. Im working on that.~~
Also, we need to hold off on merging this till core's PR is merged.

1) "Tail" option under Debug is grayed out if a file doesnt exist. No point in tailing such files. 2) Refactoring based on comments.

nangtrongvuon

Few more comments, and then I think this is in a good enough state to merge after the core PR goes in!

nangtrongvuon · 2019-12-22T06:56:43Z

Sources/XiEditor/EditViewController.swift

@@ -711,6 +717,10 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc
        document.xiCore.setTheme(themeName: sender.title)
    }

+    @IBAction func debugToggleTail(_ sender: NSMenuItem) {
+        document.xiCore.toggleTailConfig(identifier: document.coreViewIdentifier!, enabled: !self.isTailEnabled)


This is just a little nit, but I'd prefer if this was

document.xiCore.toggleTailConfig(identifier: document.coreViewIdentifier!, enabled: self.isTailEnabled)

nangtrongvuon · 2019-12-22T06:57:44Z

Sources/XiEditor/EditViewController.swift

@@ -919,6 +951,11 @@ class EditViewController: NSViewController, EditViewDataSource, FindDelegate, Sc
            languagesMenu.addItem(item)
        }
    }
+
+    public func enableTailing(_ isTailEnabled: Bool) {
+        self.isTailEnabled = isTailEnabled


.. and this part is instead:
self.isTailEnabled = !isTailEnabled

This avoids having to rely on state from another method.

See my long comment below.

nangtrongvuon · 2019-12-22T06:59:39Z

Sources/XiEditor/Core/CoreNotification.swift

@@ -228,6 +229,11 @@ enum CoreNotification {
            {
                return .replaceStatus(viewIdentifier: viewIdentifier!, status: replaceStatus)
            }
+        case "enable_tailing":
+            if let isTailEnabled = jsonParams["is_tail_enabled"] as? Bool


I think enable_tailing should be toggle_tailing instead, since it's way clearer in conveying what it does exactly. enable_tailing implies that it always enables tailing for any file, not what it actually does (which is toggle the tailing state for a specific view).

Since you changed this here, your plugin doesn't handle the old JSON properly. Perhaps you should update your other PR as well with this change?

I havent push the change. I'll do it based on outcome of the naming discussion we are having.

sjoshid · 2019-12-22T23:04:41Z

@nangtrongvuon
Allow me to add more details to the naming. I sort of struggled with it. (and Im bad at naming)
There are two parts
a) When user clicks the "Tail" option, EditViewController.debugToggleTail is the one that toggles the state (tail flag) and sends it to core. Hence the word 'toggle' in method. Tail flag belongs to EditViewController and so does debugToggleTail(). Instance method modifying instance variable. So toggling here is ok I think.
b) After sending to core, it replies with either true ie tail was toggled successfully or false otherwise. This reply from core is handled by EditViewController.enableTailing. It shouldnt be toggled because it represents the actual state of the flag. Toggling here would be incorrect. Besides, if core replies with true and I toggle in EditViewController.enableTailing, then my state would be false. Dont think this is correct either.

I still think EditViewController.enableTailing(**bool** ) is correct. Because it is not just enableTailing(). Parameter is important here. Without parameter, it means what you are saying: enable tailing. In my case it takes a bool. So I would read it as "Enable tailing. Yes or no?".

If you want me to separate enable and disable I would have to do something like

bool tailChangedInCore = <reply from core>
if tailChangedInCore
    EditViewController.enableTailing()
else
    EditViewController.disableTailing()

EditViewController.enableTailing() {
    self.isTailEnabled = true
}

EditViewController.disableTailing() {
    self.isTailEnabled = false
}

If this is what you want, I would be happy to make the change. If not, let me know what you think. Sorry about long comment. I hope Im not going off on a tangent.

nangtrongvuon · 2019-12-29T06:49:26Z

My main problem with this:

document.xiCore.toggleTailConfig(identifier: document.coreViewIdentifier!, enabled: !self.isTailEnabled)

Is that you begin to mutate state in a method's argument which I think reads really weird. It doesn't produce the wrong results, but it's also something I'm not okay with seeing in a codebase, since it makes following this code hard and also makes it less maintainable.

I'm also not a fan of using this frontend boolean to determine behavior of core. I believe this is a code smell, and core should be able to maintain which views are currently being tailed. The frontend doesn't really need to control this state, at most it should only communicate that state to the user (for xi-mac, this should be the isTailEnabled boolean), so they know that their file is being tailed and not in an editing state.

So in conclusion, I wouldn't split it into enable and disable, but I would just have a toggleTailing without passing enabled that will toggle tailing state for a view. If it worked and the state changed, cool, we should tell this to the user by changing isTailEnabled. If not, then all is fine, just keep the current frontend state.

sjoshid added 4 commits December 17, 2019 12:26

Added Toggle Tail config in Debug.

a0e36e3

Merging changes from Upstream and other refactoring changes.

cd19a96

Handling toggleTailChanged notification from core.

8a2135c

Refactoring.

63dfd41

Removing toggleTailConfigChanged added as a dup during merge.

nangtrongvuon reviewed Dec 18, 2019

View reviewed changes

Disabling "Tail" option when file doesn't exist

c746043

1) "Tail" option under Debug is grayed out if a file doesnt exist. No point in tailing such files. 2) Refactoring based on comments.

nangtrongvuon reviewed Dec 22, 2019

View reviewed changes

Merge branch 'master' into Tail_feature

5a2824f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tail feature #486

Tail feature #486

sjoshid commented Dec 17, 2019 •

edited

Loading

nangtrongvuon left a comment

nangtrongvuon Dec 18, 2019

sjoshid Dec 18, 2019

nangtrongvuon Dec 22, 2019

nangtrongvuon Dec 18, 2019

sjoshid Dec 18, 2019

sjoshid commented Dec 18, 2019 •

edited

Loading

nangtrongvuon left a comment

nangtrongvuon Dec 22, 2019

nangtrongvuon Dec 22, 2019

sjoshid Dec 22, 2019

nangtrongvuon Dec 22, 2019

sjoshid Dec 22, 2019

sjoshid commented Dec 22, 2019

nangtrongvuon commented Dec 29, 2019 •

edited

Loading

Tail feature #486

Are you sure you want to change the base?

Tail feature #486

Conversation

sjoshid commented Dec 17, 2019 • edited Loading

Review Checklist

nangtrongvuon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoshid commented Dec 18, 2019 • edited Loading

nangtrongvuon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjoshid commented Dec 22, 2019

nangtrongvuon commented Dec 29, 2019 • edited Loading

sjoshid commented Dec 17, 2019 •

edited

Loading

sjoshid commented Dec 18, 2019 •

edited

Loading

nangtrongvuon commented Dec 29, 2019 •

edited

Loading