Skip to content
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

[Textbox] Draw text after cursor #1777

Merged
merged 1 commit into from
Jan 16, 2023
Merged

[Textbox] Draw text after cursor #1777

merged 1 commit into from
Jan 16, 2023

Conversation

vE5li
Copy link
Contributor

@vE5li vE5li commented Jan 10, 2023

A couple of weeks ago my PR (#1753) was merged, making it possible to create block cursors. As part of that change I made the text render on top of the cursor so that text can still be read even when the cursor is at the same position. This change was reverted with this commit, effectively breaking block cursors again.

For reference, here is a screenshot of the current state
2023-01-10_03-01-1673319401

And here is what it's supposed to look like:
2023-01-10_03-01-1673318694

I myself don't see a reason for why the text can't be on top of the cursor so I opened this PR to fix that regression.

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Jan 10, 2023

This is getting (too) complex.
I think cursor width and block cursor is something different, normally a block cursor denotes replace mode and a normal cursor insert mode. Rofi always works in insert mode.
(Normally if an application has a block cursors, the (single) character is rendered on top of the cursor in an inverse color.)

I changed the ordering because there was some 'ugliness' with a normal cursor (non block)., must admit I did not think about the 'block' use-case (just a normal cursor).
This needs some more thought.

@vE5li
Copy link
Contributor Author

vE5li commented Jan 10, 2023

I don't think your first statement is necessarily true, at least I don't associate a block cursor with replacing. A lot of the software I use also uses block cursors for the "insert" mode. If you decide on how to handle this properly I can try to implement the new behaviour 👍

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Jan 10, 2023

I might be old school, but text entry has behaved like this for a very long time, as far back as I remember. So for me that statement feels true?
I quickly tried libreoffice and it exactly behaves like this, same for gedit and even if I google on it you find pictures like:
image

Anyway I am not sure what the best solution is here, I agree the above screenshot you posted is wrong and we should fix that one way or the other. I wonder if it is hard to instead of doing this, we invert the color of the text that goes over the cursor.

thanks for your feedback and patches, it is much appreciated.

@vE5li
Copy link
Contributor Author

vE5li commented Jan 10, 2023

Ah fair enough, I misunderstood what you were trying to say. The block cursor I am talking about purely replaces the simple line cursor with a thicker one. That is what many other applications do too (most terminal emulators and text editors have this feature) and I personally find it to be much more aesthetically pleasing.

@DaveDavenport
Copy link
Collaborator

I don't think we are mixing anything. I understand you want a block cursor, but for me (and some others (not all) I've talked to) a block cursor is a visual indication that you are in overwrite mode.
I guess it boils down to preference, I myself don't like how terminals (and vim on some terminals) do it, as it is giving me a contradicting visual hint from what I've been used too since forever.

So as I mentioned before, we should fix the rendering. For me the question is (given this not been in a release we can still overhaul it) what is the best solution that in general gives the best result.
I myself kinda like the idea of rendering the text, render the cursor, add a clip where the cursor is, render the text again (clipped) in inverse color to the cursor.. This should allow for the best contrast.

I've recently added on request the text outline, I think I would not even mind 'sacrificing' this for having nicer rendering of this.

@vE5li
Copy link
Contributor Author

vE5li commented Jan 10, 2023

Sorry if my previous reply came off as rude, that was not my intention. I fully agree with what you are saying about the rendering, although I don't know if I am able to implement the logic you suggested. Might be that someone else has to take over at this point.

@DaveDavenport
Copy link
Collaborator

No problem, I rather have a discussion and concluding we might not agree, then have no discussion at all.

@DaveDavenport DaveDavenport merged commit 8155b2c into davatorium:next Jan 16, 2023
@DaveDavenport
Copy link
Collaborator

Lets swap it back for now, and Ill maybe rewrite the whole drawing in a later version.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
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