search

xi-editor / xi-mac

The xi-editor mac frontend.
Apache License 2.0
3.02k stars 147 forks source link

Tail feature #486

Open sjoshid opened 4 years ago

sjoshid commented 4 years ago

Closes https://github.com/xi-editor/xi-editor/issues/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

sjoshid commented 4 years ago

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.

sjoshid commented 4 years ago

@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 commented 4 years ago

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.