-
-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show Settings Icon in Status Bar #421
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving because: I tested it and it's working.
Code overall looks reasonable.
I note that toggling the setting requires a new window or restarting the app/current window to take effect, but I see that as totally reasonable and okay.
It might be possible to do statusViewIcon = new SettingsIconStatusView(statusBar)
outside of consumeStatusBar()
, and then it could be destroy()
ed when the setting is toggled, or perhaps only when deactivating the package? But honestly, I think that might be overkill, and the extra code complexity might not being worth how slick/cool this would seem, toggling the setting and the icon just goes away, would be "neat".
At the moment I assume(?) the destroy()
function in packages/settings-view/lib/settings-icon-status-view.js is never used, but having the ability to easily use this functionality in the future makes it worthwhile to have.
Just looking for things to say here honestly, but in all honestly the main bit of what I have to say is: "tested working, looks good to me."
P.S. (EDIT): I like that it is toggleable with a setting, some folks might consider it clutter, others might consider it super convenient to have a GUI-oriented, "point-and-click with the mouse" way to get into settings. So, having it be a preference is great, IMO.
I'm definitely one of those, I've been using |
I am intending to merge this before we do a 1.103 Regular release, if someone else hasn't done it before then. Leaving it for a moment in case anyone's going to leave a review, since those don't really work after a merge, but yeah, I'm ready to merge this one for the next release. |
I do like the idea of included some changes to make it toggle right away. But if alright by you might be work for another day. And as for now with approval I'd be more than happy to merge this one |
I've noticed a couple of things about this implementation - one of which I've already submitted a PR for at #456 The other is more... up for debate... as I have no idea what the correct answer is. However this seemingly goes much higher and can be bested by other packages, for example So if we wanted to ensure this always shows on the very far right then we need to set an even higher (lower???? cuz -ve) priority for the icon to make sure it trumps most others. However I imagine that can still lead to competing until it hits some kind of integer overflow. The other solution is that we somehow hard code this to always be on the right regardless of other packages, like giving it its own section of the panel to which the rules do not apply. Related to this I think that there should be a very easy to access user override for the panel icons, either via a simple config file heirachy or drag-and-drop. i.e. not just making people do it in the init script, it should be for all levels. |
@Daeraxa That's good to know it can go higher. Personally I'm not a huge fan of priority system, and totally agree it'd be nice to let it be easily configurable, but doing so would likely need a pretty big refactor of how the icons are handled, and I'm not sure that's worth doing just for this one icon. Like if we could make a system that uses the priority, but then allows a user to change it, and stay with that change that'd be awesome, but what we could do for now is one of two things either set the priority here to |
No, I definitely agree, I'll open the idea as an enhancement issue.
Did a quick check and |
So maybe if we want to always ensure it's right aligned we can set it to the negative max safe integer, because then it'd be impossible for another package to override it. Even if I'm not a huge fan of condoning that sorta behavior |
I agree, it seems wrong somehow which is why I was bringing it up as a discussion rather than just submitting a |
I think ultimately the right answer is making it easily user-configurable. I don't think it looks too odd in that screenshot, but I also think any system where various packages have to argue about the order in which they appear on the status bar is doomed to failure. |
Simple PR, adds the a settings icon into the status bar.
Within
settings-view
further utilizes thestatusBar
service, which it already consumes.When clicking it will open up the settings via the
atom.workspace()
API.Resolves #313
Additionally this is totally configurable. By default displaying the icon, but can always be turned off via
showSettingsIconInStatusBar