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

Added the ability to force text direction of entries #62

Merged
merged 2 commits into from
Jan 8, 2021
Merged

Added the ability to force text direction of entries #62

merged 2 commits into from
Jan 8, 2021

Conversation

reeseovine
Copy link
Contributor

@reeseovine reeseovine commented Dec 21, 2020

Why

There are many reasons someone might want this feature. Maybe you want entries to flow in your language's natural reading order. Or maybe you just want all the characters to be in line with each other vertically. Also, rofi doesn't provide a way to justify text natively and this is the recommended method to do so.

How

This adds a special control mark (depending on how you set the option) before every line when reading files. After an entry is returned this mark is stripped and the program continues normally. Alignment is also applied to the recent characters list and the skin tone selection menu.

The control marks are described in the table below. All of them have zero width so they will not add anything. The purpose of the non-breaking space is to keep the add/strip logic simple.

code description
U+200E Left-to-right mark
U+200F Right-to-left mark
U+FEFF Non-breaking space

@reeseovine reeseovine marked this pull request as ready for review December 21, 2020 08:17
@fdw
Copy link
Owner

fdw commented Dec 22, 2020

Hey @katacarbix , thanks for this PR 🙂

Just to manage your expectations, though, I don't think I'll have time for a review in the next days, sorry.

@reeseovine
Copy link
Contributor Author

That's okay, no rush!

@fdw fdw mentioned this pull request Dec 28, 2020
@fdw
Copy link
Owner

fdw commented Dec 28, 2020

First off, as always: Thank you 🙂 I think this could be a good addition, especially with the problem in nerd fonts @polyzen mentioned in #64.

I've had a look around and thought some time about the whole thing, and these items came to mind:

  • Adding a character to each line is probably a bit expensive computation-wise, so we should be careful to only use that when necessary (so likely not for auto).
  • The actual addition of the character should not happen in the "file read"-methods, but in some separate pre-processing step, just to make the code better read- and understandable.
  • When removing the direction characters again, we must make sure to get all of them - you can select several characters at once (each with their own direction char), and currently only the first is stripped away.

Do you want to update your code, or should I, when I find time and inspiraton? 😉

@reeseovine
Copy link
Contributor Author

Yes, that problem is the main inspiration for this PR.

To answer your concerns:

  1. I agree, I was being a bit lazy when I did that. My main concern was that it would make the code ever so slightly harder to read when adding a condition to the logic, but now that I think about it I don't see an issue with it.
  2. You're right, I do think that would make it more readable. I put it in that step because I thought there may be a performance impact if I added another loop. I'm not exactly sure how to time it but I think that would be an important step to weigh the options here.
  3. I was actually unaware you could select more than one character. I think I may need some help with that part.

@reeseovine
Copy link
Contributor Author

First 2 issues are fixed, still need to look into the last one.

@fdw
Copy link
Owner

fdw commented Dec 29, 2020

Thanks for the changes, it looks better now 🙂

I still have some more ideas for improvements 😉, but now that I've played a bit more with it, I found a few other problems that make me question the approach (not the goal itself, mind you!).

First, when using rtl there are several characters which are not on the right side but only right aligned (almost all flags, for example):
rofi-2020-12-29-0943-00000
This is weird, and I don't know why they behave like that.

But more importantly, I wonder if rtl actually ever makes sense: If you use such a language, having the icon on the right is nice, but the description is still in a ltr language (English), thus you need to parse it from the left. If the entry is now right aligned, that is actually harder than before.
If the description were localized, rtl would make more sense but for now all you get is English, so I think there's no point in currently having rtl.

Building on this, the only problem this PR now solves is aligning all entries to the left, which would be an improvement. But I have a problem here, too:
rofi-2020-12-29-1004-00000

The letters are now left-aligned, and that is better than before. However, the niqqud (in the left column) are harder to see now compared with before:
rofi-2020-12-29-1006-00000

The reason is that they are additions to other characters. On their own, they are rendered (at least on my system) with this helpful circle. If a ltr-marker is prefixed, they seem to be merged with it. So either we need another workaround here (and I don't know what that could look like), or we need another approach.

Which brings me to the idea @polyzen mentioned over in #64:

Perhaps the extractor should automatically apply the left-to-right mark to the affected characters in nerd_fonts.csv.

Since we only want left-aligned (I'm assuming that now 😉), we could just add the marker in the data files, avoiding a lot of work for rofimoji. If we can also find a way to only add the marker where it's needed, that sounds to me like a good solution.

Lots of text. What do you say?

@reeseovine
Copy link
Contributor Author

Thank you for finding these, I must not have tested it thoroughly enough. I'm very much open to trying a different route.

If we add a mark only to the rows that need it during extraction, how would the picker know when to strip the mark from the entry once it's selected? It could check whether the first character is a direction mark, but it would need to account for selecting an actual direction mark for example, and I think at that point the code might get kind of complicated. I appreciate how readable the code is and would like to keep it that way.

One potential solution that has been floating around in my head is to convert the data files to actual comma separated values (or tabs since there are a lot of commas in the emoji attributes). That way we could add an optional third column to specify a forced direction. I'm not sure if that would make the logic to strip them after selection any simpler though. Using tabs would remove some of the ambiguity that comes with separating by spaces or commas. If we do that we would need to escape the literal tab character and the escape character itself in any entries that include them.

This idea came from splatmoji which looks like a similar idea to rofimoji but is not as featureful. They also have emoji lists in several languages so I think that could be possible for us to do that as well.

Changing the data format would obviously be a breaking change (unless we kept support for both), but let me know what you think and I can try implementing it.

@fdw
Copy link
Owner

fdw commented Dec 30, 2020

Changing the file format to something that needs to be parsed would, again, come with a performance penalty every single time rofimoji starts. I'm willing to talk about this for a huge benefit, but I don't see any at the moment.

What I would suggest is to somehow add the direction marker - if needed - in the extraction step.
For the picker itself, we can remove the direction markers with a regex (that would also solve the problem of multiple selected characters). Since the direction markers aren't contained in the data files (yet), that would not be a problem. If they should be, we can find a way to escape them so that the regex keeps working.

@reeseovine
Copy link
Contributor Author

Hey sorry for the delay, I had to take some time for life stuff. Happy new year by the way!

I am able to add marks before the lines that need them (see last commit) but I'm having a bit of trouble understanding how to strip them after selection. The regex substitution that I came up with and used in __init__() is this:

re.sub(r'^[\u200e\u200f](?!\s)', '', stdout)

My problem is that it doesn't seem to work with multi-line selections. I could use a bit of help with that part of it.

Also, once you are happy with it I can revert the code for the original method (if you'd like) and squash everything into 1 commit.

@fdw
Copy link
Owner

fdw commented Jan 5, 2021

Hey sorry for the delay, I had to take some time for life stuff. Happy new year by the way!

Happy new year to you, too! No worries about the delay, we all have more important things than a character picker 😉

My problem is that it doesn't seem to work with multi-line selections.

At a glance, I think this is because of the ^ in the regex - that matches the beginning of the line. With multiple characters selected, the string looks like \u200esomething something\n\u200esomething something. So, one way is to remove that ^.

Alternatively, I think a good place to do that processing would be process chosen characters, where you have a List[str] and where your regex should work again.

Also, once you are happy with it I can revert the code for the original method (if you'd like) and squash everything into 1 commit.

Thanks, that would understanding the history easier.

Finally, if this works, I think we should also add it to the other extractors.

@reeseovine
Copy link
Contributor Author

reeseovine commented Jan 5, 2021

Done! I made it so that every line is matched and grouped by the regex rather than using .split(' '). I don't think it should have any performance impact, and it actually fixes a bug where the entry for Space doesn't enter anything (not that anyone would really need it...)

I'm confident that this solution works but it could definitely use more testing.

picker/rofimoji.py Outdated Show resolved Hide resolved
@fdw fdw merged commit 7579a6a into fdw:main Jan 8, 2021
@fdw
Copy link
Owner

fdw commented Jan 8, 2021

Thank you, again! 🙂

@polyzen
Copy link
Contributor

polyzen commented Jan 8, 2021

Seems there are a few characters missing the fix, eg. nf-mdi-webhook.

@fdw
Copy link
Owner

fdw commented Jan 8, 2021

You're right, but it should be fixed now 🙂

@reeseovine
Copy link
Contributor Author

Awesome, and thanks for all the help!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants