Notes

[ian-r-rose]

Hi all, got a chance to play around a bit with the status bar. Looking good! A few design/behavior thoughts that I had:

• Not all of the statusbar items are clickable, but all of them seem to have some hover styling. Maybe only the ones that are clickable should highlight on hovering.
• Is the kernel symbol a corn kernel? That's clever, but I did find it a bit confusing, since that symbol isn't really used elsewhere.
• The "tab spaces" indicator didn't update when I changed the settings in the settings menu. It should probably listen to the settings.changed signal for the CodeEditor settings.
• I thought difference in border width between the left area and the main area looked a little strange:
  Not sure how to fix it though.
• The active document item is nice. I think it could also be extended to other widgets like the launcher or settings editor.
• The "go to line" dialog is also nice. It should check that the input is a number, though. I was able to produce errors by entering characters.

Nice work!

[declanvk]

@ian-r-rose Thank you for the review!

[ian-r-rose]

A few code review notes:

• In SignalExt.combine you are connecting to a couple of signals. I think it should also have some way of disconnecting those. I am unsure what the best approach is right now.
• I think the IDefaultsManager is kind of confusing. It seems to me it is more like an IToolbarManager, where you distribute a series of default items that are added to it. That is to say, if a third-party extension author wanted to add a new toolbar item, do they use the IDefaultsManager, or the IStatusBar? What is the intended use of the of the IDefaultsManager as a token for extension authors to use?
• In the FilePath status item you are checking if an object is an instanceof a DocumentWidget. A safer way of checking for this is checking documentManager.contextForWidget(), which will let you know if a given widget is known by the document manager.
• Similarly, you are doing instanceof checks in the tabSpace and lineCol items. However, you have a better way of checking the types of the main area widgets: you have all three instance trackers which know whether a given widget belongs to them. So you can write getFocusedEditor so that it can check the actual instance trackers, and then more reliably get the editor widget.
• The same thing goes for the kernelStatus item and their sessions (can you tell I really don't like instanceof checks?).
• Is it necessary for the Status items to provide a string id? I didn't really see it being used for much, other than checking whether a status item is added twice.

Overall I really like the architecture you have set up here! I'll take a closer look later.

[declanvk]

Hey @ian-r-rose thanks again for another review. We've addressed your points with some PRs and some explanations:

• With the respect to the Signal combining stuff I have Add CombinedSignal extension #60 which creates a class which will disconnect from multiple parent Signals on dispose. If you want to take a look at this before we merge it, any further feedback would be welcome.
• The IDefaultsManager was not intended for use by third-party developers. Its main function is in checking the settings registry against the ids of the items added to it, so that end users have the ability to disable the default status items. We could make it into a more general settings connector that performs that same function for all items.
• Richa has fixed up instance of instanceof with Instance of #61.
• In a similar vein, the use of the string id is mostly for use in the settings registry. We are considering getting rid of it for non-default status items, but that would depend largely on changes to the IDefaultManager.