Skip to content
This repository was archived by the owner on Apr 6, 2018. It is now read-only.

:feature: show submode in status bar#574

Merged
maxbrunsfeld merged 6 commits into
atom:masterfrom
jacekkopecky:status-bar-with-submode
Mar 15, 2015
Merged

:feature: show submode in status bar#574
maxbrunsfeld merged 6 commits into
atom:masterfrom
jacekkopecky:status-bar-with-submode

Conversation

@jacekkopecky
Copy link
Copy Markdown
Contributor

This is a suggestion that we may want to show the submode in the status bar; the PR already includes values for replace mode, proposed in #573.

@jacekkopecky jacekkopecky mentioned this pull request Mar 12, 2015
Comment thread lib/status-bar-manager.coffee Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it; nice solution. Nitpicks:

  • I'd rather use . as the delimiter than $. It would just mean we'd have to quote the keys.
  • I think you could avoid depending on the order of ContentsByMode if you did this:
currentMode = currentMode + "." + currentSubmode if currentSubmode?
for mode, [klass, html] of ContentsByMode
  if mode is currentMode
    # ...  

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
I've committed a fix in 91069db so the code doesn't rely on order, keeping the ability to match only on the mode. If we don't keep currentFullMode we'd have to enumerate all submodes in ContentsByMode.
Which alternative would you prefer?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see what you're saying. I think the most straightforward thing to do is to use the code snippet I wrote above; we just need to enumerate all possible combinations of mode and submode in ContentsByMode, rather than abbreviating visual.characterwise as visual.

I don't think we can use a break statement like you have on line 30, because it could incorrectly prevent classes from being removed from the @element div.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I missed that. OK, enumeration it is; the longer I've thought about it the better this option has sounded anyway. Commit coming soon.

@jacekkopecky jacekkopecky force-pushed the status-bar-with-submode branch from 91069db to 0098383 Compare March 13, 2015 21:09
the previous version could leave the wrong classes on the status bar div
@jacekkopecky
Copy link
Copy Markdown
Contributor Author

And I still can't get it as simple as you suggested, @maxbrunsfeld :
Since multiple entries in ContentsByMode share the same class name, there was a risk that a later entry with the same class name but not a matching mode would remove the class set by a preceding entry with a matching mode. In other words, in visual.characterwise, the @element would not have the class status-bar-vim-mode-visual because visual.blockwise would remove it.
So 5490b33 does things differently still. Comments welcome.

maxbrunsfeld pushed a commit that referenced this pull request Mar 15, 2015
@maxbrunsfeld maxbrunsfeld merged commit 90f30c1 into atom:master Mar 15, 2015
@jacekkopecky jacekkopecky deleted the status-bar-with-submode branch March 15, 2015 16:24
@jacekkopecky
Copy link
Copy Markdown
Contributor Author

Thanks! 8-)

MarkusSN pushed a commit to MarkusSN/vim-mode that referenced this pull request May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants