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

Add Status bar Indicator for Overwrite mode #6670

Conversation

WebsiteDeveloper
Copy link
Contributor

@larz0 for UI review.

Please notice the ability to indicate the current toggle state when changed via insert key is depending on codemirror/codemirror5@f9aa052 to be include. But it can be changed via clicking on the status bar item.

@larz0
Copy link
Member

larz0 commented Jan 29, 2014

@WebsiteDeveloper could you attach a screenshot? Don't have a laptop with me right now.

@WebsiteDeveloper
Copy link
Contributor Author

2014-01-29 07_29_41-index html - brackets
2014-01-29 07_29_18-index html - brackets
@larz0 uploaded.

@larz0
Copy link
Member

larz0 commented Jan 29, 2014

I'm wondering if we could just make it toggle between "insert" and "overtype" like the tab/space toggle without the green background. Overtype mode is pretty obvious and requires user activation first so we probably won't need to call it out like that.

If for some reason a background color is needed then we should use #78B2F2 (blue) for the active state, keeping the gray border on both sides.

Thoughts?

@lkcampbell
Copy link
Contributor

@larz0, the original bug was about people accidentally hitting the Ins key on Windows and put Brackets into overwrite mode without knowing they did it. So, while toggling the mode via the status bar is more obvious, toggling the mode by accidentally hitting the Ins key is not obvious at all. I'm not sure a green background is the best way to do it, but something should be done to bring the toggled state to the user's attention.

@peterflynn
Copy link
Member

Personally I don't mind calling it out strongly, but I think there's also a reasonable argument for not doing that: in every other editor the statusbar indication is quite subtle, and the cursor appearance change is a more direct indicator (in context) anyway.

@larz0
Copy link
Member

larz0 commented Jan 29, 2014

@lkcampbell I see. So either the behavior of the cursor input isn't obvious or the user don't know what activated overtype mode. (Is this common on Windows?)

If that's the case then let's use the blue background on the indicator.

@lkcampbell
Copy link
Contributor

@larz0, actually, @peterflynn has a good point. I didn't think about the cursor change. That is a good visual indicator.

@TomMalbran
Copy link
Contributor

I think it might look good to just change the color of the text "OVW" from a light gray (deactivated) to black (activated).

@larz0
Copy link
Member

larz0 commented Jan 29, 2014

@lkcampbell ok cool.

@TomMalbran let's change the background color if it's absolutely as it's a little heavy-handed, what @peterflynn mentioned above.

@larz0
Copy link
Member

larz0 commented Jan 29, 2014

@TomMalbran we could do that so it's more obvious for the Mac users that overtype feature is available.

@redmunds
Copy link
Contributor

I think changing the background color might be too much. Visual Studio displays "INS" in insert mode and "OVR" in overwrite mode. They also change the cursor.

@WebsiteDeveloper
Copy link
Contributor Author

Is there some kind of consensus what to do?

@le717
Copy link
Contributor

le717 commented Jan 31, 2014

I think changing the background color might be too much. Visual Studio displays "INS" in insert mode and "OVR" in overwrite mode. They also change the cursor.

Notepad++ does the exact same thing.
Insert:
npp-insert
Overwrite:
npp-overwrite

Looking back at the issue this PR fixes (#6206), they are wanting some visual feedback on what mode is enabled (as most are suggested be done). #4250, on the other hand, suggested the same thing @redmunds is.

Personally, I think either solution would work and solve the issue here. A small area at the bottom of your screen suddenly turning blue would catch pretty much anybody's eye, and it gives near-instant feedback on what mode is activated. Simply changing the cursor to an _ and the label is practically what everybody does to illustrate the two modes.
@TomMalbran's suggestion to

...just change the color of the text "OVW" from a light gray (deactivated) to black (activated).

doesn't provide too much feedback, and it can be hard for some people to discern between the two semi-subtle color changes (like myself. I am slightly color-blind, and the black/dark gray area is one of trouble spots).

You could say the answer is summed up in one question: do you want to do the same as everyone else, or do you want to be different and roll your own way?

@redmunds
Copy link
Contributor

Another idea: give the indicator a background-color that fades away after a few seconds. That way, you'd notice when it changes, but it wouldn't always stand out.

@lkcampbell
Copy link
Contributor

Like @le717 said, traditionally, the abbreviations I have seen in editors are INS and OVR. The toggle flips the abbreviations. I suspect this would be something that might need to be translated into appropriate abbreviations in other languages as well.

I've never seen OVW used before. Is that from an editor I haven't seen before?

The cursor shape change is a really good indicator and I also really like @redmunds idea as well, a flash of background color that fades to catch the user's attention.

@redmunds
Copy link
Contributor

Is there some kind of consensus what to do?

@WebsiteDeveloper We need to wait for @larz0 to make a decision. Note that he's on vacation so may be slow to respond.

FYI, I'm waiting for you to respond to my comments on related pull request #6328 if you want to look at that one.

@le717
Copy link
Contributor

le717 commented Jan 31, 2014

@redmunds @lkcampbell A quick flash of color/possibly label change plus the cursor sounds like the best of both ideas. You get the sudden visual indication and a more subtle constant reminder. You'd just have to make sure the blue background stayed up long enough for the user to understand what it means.

@le717
Copy link
Contributor

le717 commented Jan 31, 2014

... But if that is chosen, then an idea for switching back to insert mode needs to be devised, probably along the same lines but using a different color/label change.

@larz0
Copy link
Member

larz0 commented Feb 1, 2014

@lkcampbell +1 for INS/OVR toggle. We could use white text on #78B2F2 for the flash.

@WebsiteDeveloper
Copy link
Contributor Author

@larz0 @redmunds pushed changes. waiting for your opinion.

@redmunds
Copy link
Contributor

redmunds commented Feb 1, 2014

@WebsiteDeveloper The INS indicator doesn't change for me. I expected it to change when I press "Insert" key on Windows or if I click on it in Statusbar.

What do you think about combining both Insert Mode pull requests (this one and #6328) into a single branch and pull request so we can see how it all works together?

@marcelgerber
Copy link
Contributor

@redmunds You need codemirror/codemirror5@f9aa052 to try this PR, I had to do the same.

@WebsiteDeveloper I don't know if @larz0 meant to flash the indicator on every toggle, I think it should not stay blue.

@marcelgerber
Copy link
Contributor

@larz0 Quick screencast:
statusbar_overwrite

In OVR mode, it stays blue until you toggle into INS mode.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds lets wait until @larz0 gives his opinion. Once the feature is finished i think it's a good idea to merge both branches.

@larz0
Copy link
Member

larz0 commented Feb 1, 2014

@SAplayer thanks for the screencast. Looks good!

@larz0
Copy link
Member

larz0 commented Feb 1, 2014

The blue should also fade out in overtype mode.

@WebsiteDeveloper
Copy link
Contributor Author

@larz0 @SAplayer pushed changes.

@marcelgerber
Copy link
Contributor

Just another screencast for @larz0:
statusbar

@redmunds
Copy link
Contributor

redmunds commented Feb 2, 2014

I think switching the text color between black and white during highlighting makes it hard to read. I think text should always be black. If that makes it hard to read against dark background, then maybe background shouldn't be as dark, or text should have a light text-shadow.

@larz0
Copy link
Member

larz0 commented Feb 2, 2014

Ok let's keep the text black to keep it simple.

@marcelgerber
Copy link
Contributor

@WebsiteDeveloper I think you could also use AnimationUtils.animateUsingClass($statusOverwrite, "active") instead of the setTimeout() solution?

function _updateEditorOverwriteMode() {
var editor = EditorManager.getActiveEditor();

editor._codeMirror.toggleOverwrite(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a new Editor.toggleOverwrite method which just does this._codeMirror.toggleOverwrite(value), so that we don't access the private member _codeMirror?

@WebsiteDeveloper
Copy link
Contributor Author

@SAplayer i didn't know that module existed^
Always learning something new 👍

@WebsiteDeveloper
Copy link
Contributor Author

@SAplayer @redmunds @larz0 pushed changes.

@@ -921,6 +924,15 @@ define(function (require, exports, module) {
};

/**
* Sets or toggles the editors overwrite mode.
*
* @param {!boolean} start
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be @param {?boolean} state since you can pass null too. Might be nice to have a description that you can use a boolean value to set and null to toggle the state.

Also, you could the empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh forgot that you're right

@marcelgerber
Copy link
Contributor

Fwiw, a screencast for @larz0 again:
statusbar

@marcelgerber
Copy link
Contributor

@WebsiteDeveloper Should we maybe rename the class active into animate or flash or something?
That'd make more sense as the active state is no more persistent.

@larz0
Copy link
Member

larz0 commented Feb 4, 2014

Looks good! Thanks!

@WebsiteDeveloper
Copy link
Contributor Author

This pull is superseded by #6777

@JeffryBooher
Copy link
Contributor

@WebsiteDeveloper Please close this PR if if is superseded by #6777

@WebsiteDeveloper
Copy link
Contributor Author

oh forgot that

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.

9 participants