-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Updated configurable keys #788
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
Conversation
Might help new users.
Otherwise we get: AndroidRuntime: java.lang.NullPointerException: Attempt to invoke virtual method 'boolean android.widget.CompoundButton.isChecked()' on a null object reference AndroidRuntime: at com.termux.app.ExtraKeysView.b(SourceFile:128)
|
Fixes #749. |
|
Very nice ! Thanks ! I was just about to start coding the PR myself. But I prefer to think about the functionalities and summarize them as I did. Giving good input or good specs and let experts implement the feature. 😄 |
That answers my question I had in the other PR. So yes, validation or default behavior, choose wisely. As goes the saying having good defaults is important, so is configuration. For the case two lines of different length I'd say no error, it means all width = 1 (see my definition of width in the other PR), it's alright not to have a grid, our real hardware keyboards have hex layouts. 😄
Well, choose wisely the good defaults ! I think the people that don't know about configuration (beginners?) prefer the ↑↓ arrows as mandatory for a shell and don't care ←→ are very nice too but less important. Beginners don't know about the configuration until we don't write a good settings menu. The dash "-" and "/" is convenient but easily found on all the keyboards. The "|" is less used by beginners (and again found on all the keyboards). We should focus the default to be on missing keys of classic android keyboard, but that are still widely use. ←→↓↑ are cleary it. And the 4, the Alt is useful for people knowing that emacs basic shortcuts like For the one line order, I think "←→↓↑" is a good choice, because the most useful key is at the right. "←↓↑→" is my second advice because it's VIM like.
It's JSON isn't it ? What kind of format termux.properties is made of ? I see you use single quote but they are not allowed in JSON, so is it a typo?
👍 And there is the wiki.
I've thought about this feature, it's only useful if it's explicitly written in the termux.properties. Nobody would think of slide-up if they're not told to. People try to press, sometimes to Long press, never to slide up, that's weird. However, if someone reads you can write
Again, what's the specification format for termux.properties ? If there is none, I suggest :
Maybe your encoding is not correctly set on your machine, change all your tools to utf-8 I'd say. 😄
They do, on your screen, on mine they are less tall than that #HopeThisHelps |
| break; | ||
| case "-": | ||
| chars = "-"; | ||
| break; |
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.
Could be refactored:
case "−":
case "-":
// if it's "−" or "-", it's the same output
chars = "-";
break;Including the comment, not so much people know the syntax.
| case "↓": | ||
| keyCode = KeyEvent.KEYCODE_DPAD_DOWN; | ||
| break; | ||
| case "↲": |
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.
Add case "ENTER": too, of course the displayed chars must be "↲" but if people write ENTER in termux.properties it should work as expected.
| case "PGUP": | ||
| keyCode = KeyEvent.KEYCODE_PAGE_UP; | ||
| break; | ||
| case "PGDN": |
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.
Add aliases, PAGE_DOWN, PAGEDOWN etc. People are likely to have that in their termux.properties.
Same for INSERT, DELETE, BACKSPACE, UP DOWN LEFT RIGHT etc. A short name that is the preffered one, and longer name explicit.
| } | ||
| if (longPressCount == 0) { | ||
| if (popupWindow != null && "―/".contains(buttonText)) { | ||
| if (popupWindow != null && "―/-".contains(buttonText)) { |
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.
In the case we have buttonText.length != 1, I would go for:
Arrays.asList("―", "/", "-").contains(yourValue)| break; | ||
| case "-": | ||
| chars = "-"; | ||
| break; |
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.
What if the guy wrote "\u2015" in the termux properties? We should remove that case, if he wrote "\u2015" it means he wants to write U+2015 ― HORIZONTAL BAR. Not U+002D - HYPHEN-MINUS.
The default config should contains the ascii dash (U+002D - HYPHEN-MINUS), but should be displayed as "−" (U+2212 − MINUS SIGN) in the shell. See my display parameter in the PR.
|
After reading the source code However, also considering JSON has some advantages. This Because new lines must be escaped: Which is annoying for that case. The same information is available in JSON: I suggest that the code looks up for The only advantage is that one doesn't have to put quotes around all the keys and values for simple cases like we generally have. Another advantage is that Comments are allowed. And that new lines don't have to be written "\n". However everybody does know Json, less people know java.util.properties but the syntax is so intuitive that's alright. Both syntaxes allow \u1234 syntax, neither does allow \U12345678 syntax to input emojis by unicode. One option would be to be more lenient on the json file:
We could switch to YAML, but less people know about it. JSON is simple but more verbose. |
I'm definitely no java expert though, but hey, thanks. So about the termux.properties file. As you've already said it's just a normal utf-8 file, which is parsed by java.util.Properties into a properties object. The extrakeys entry is read into a string which is later parsed by the JSONArray library into a json object. So the string must have a syntax that JSONArray can handle. ' instead of " works just fine. The reason I mentioned utf-16 is because java seem to use utf16 for strings, so maybe java.util.properties reads the utf-8 file as utf-16 and causes the problems. I don't really know, have to do more testing. Anyways, good that the development is on it's way! |
Actually |
|
I might fork the repo and add commits to the pull request, if I find how to do do that, do I have to do another PR? Or can I join in? I know git and branches but pretty new to github.
Cmon, a little bragging doesn't hurt, and expert doesn't have a good definition (this word makes me think about this humorous video).
And in the source code.
Indeed, java does use UTF-16, And the default keys should be ascii. The user can write strings like "LEFT", "RIGHT", "UP", "DOWN" for Arrow keys and control characters. That would be Displayed as ←↑→↓ (↲ Enter) (↹ Tab). And if the user doesn't want that he can use the "display" property I was thinking of: extrakeys = [[{"key":"TAB", "display":"TAB"}, {"key":"CTRL", "display":"^"}]]Or we can add a new option: extrakeys = [["TAB", "CTRL"]]
extrakeys.controlcharstext = ["TAB", "ENTER"]This would mean, these keys shall be rendered as "TAB" and "ENTER" and not their default (↲ Enter, ↹ Tab) but I prefer the "display" per key. |
Sounds great! I probably won't work on this again for another day or two.
Yupp, I checked, it's 3 characters. So we need to decode and encode it as utf-16 instead in ExtraKeys.java I guess. I don't see how # coding: utf-8 would help (and I tried adding it to termux.properties, no difference). You have lots of good ideas that could probably be split into several PRs, which this first one aiming at making it possible to configure keys (without breaking termux easily) and then enhance it by increasing user-friendliness and adding more options and possibilities. |
|
Okep, I'll pull request You then. Indeed ^[ \t\f]*#.*?coding[:=][ \t]*([-_.a-zA-Z0-9]+)
Yep of course. The PR serves also as a conversation, We Could talk about the ideas in a Issue though, then go for multiple PR tagging that issue. However, the first version of the implementation should be thought, not just this sounds good to me, here's my PR and fornwall would be like This is great, I'll push it on the store aaand the new user will complain Why do you put two bars and no configurable keys. Okep, back to coding, however I currently can't test the app myself (pretty new to android native app), how do I do that? 😄 At first I'll do the Java-Only parts. |
…rs for special keys (will be easily changed in the properties after)
|
For the encoding issue, in try (FileInputStream in = new FileInputStream(propsFile)) {
props.load(new InputStreamReader(in, "utf-8"));
}Instead of The |
|
Alright, I just added two commits, but I didn't test it because I haven't installed the android sdk yet. If you could compile and test for me that'd be good.
|
|
I'll test your changes and give you a report :) About building the app, you can install the sdk (and ndk) by running scripts/setup-android-sdk.sh in termux-packages. You also need to have java installed (obviously). After that, you can build the app pretty easily from the command line (without the need of installing android-studio). I have cloned all the termux apps to a common folder and from that folder I run this script: #!/bin/sh
### passwords for keystore
# keystore_pw=$(pass dev/keystore)
# key_pw=$(pass dev/keystore-termux)
### your keystore file
# keystore_file=NAME.keystore
export ANDROID_HOME=~/lib/android-sdk
export ANDROID_NDK_HOME=~/lib/android-ndk
if [ -z $1 ]; then
apps="app api float tasker styling boot"
else
apps="$1"
fi
mkdir -p apps
for app in $apps; do
if [ ! -d "termux-$app" ]; then
git clone git@github.com:termux/termux-$app
fi
cd termux-$app
### Name the apk after the branch
branch=$(git rev-parse --abbrev-ref HEAD)
./gradlew clean
### Build with gradle:
./gradlew assemblerelease
### Align the app (not strictly necessary)
zipalign -v -p 4 app/build/outputs/apk/release/app-release-{unsigned,aligned}.apk
### Sign the app:
# jarsigner -sigalg SHA1withRSA -digestalg SHA1 \
# -keystore ~/.android/${keystore_file} \
# -storepass ${keystore_pw} \
# -keypass ${key_pw} \
# app/build/outputs/apk/release/app-release-aligned.apk termux_app
#### Move the finished apk to folder "apps"
cp app/build/outputs/apk/release/app-release-aligned.apk ../apps/termux-$app-$branch.apk
cd ..
doneIf you want to sign the app as well you need to generate a signing key and then un-comment those things in the script. Remember that you can't have several termux apps installed if they are signed with different keys, i.e. you need to uninstall termux-{app,api,...} fully before installing your own version, so remember to backup $HOME if you do this. |
Basic configuration of extrakeys and arrows for a lot of characters
Fix refactoring, Ctrl, Alt, Fn keys work again
|
Oh and don't forget to change the U+2015 ― HORIZONTAL BAR into a ascii dash: - JSONArray arr = new JSONArray(props.getProperty("extrakeys", "[[\"ESC\",\"CTRL\",\"ALT\",\"TAB\",\"―\",\"/\",\"|\"]]"));
+ JSONArray arr = new JSONArray(props.getProperty("extrakeys", "[[\"ESC\",\"CTRL\",\"ALT\",\"TAB\",\"-\",\"/\",\"|\"]]"));For the default one line bar. @Grimler91 I just added a commit with that change and renaming to When you say cherry pick my commit, you mean executing |
|
About JSON version, the From the doc, single quotes are normal, and often don't need to even used ! The texts produced bfy the toString methods strictly conform to the JSON syntax rules. The constructors are more forgiving in the texts they will accept:
So the default can be written: The last three don't have to be escaped, but I prefer it like this. That's the correct spelling: They also have a |
|
I've invited you to collaborate (you should have an email from github) to my grimler91/termux-app fork. I merged in changes from master and then cherry-picked your commit. With cherry-pick I mean
I think that look more confusing, better to quote all strings then, even if it isn't strictly needed. Wait, "\\" is for one backslash? Wow... An alias would indeed be nice then! |
|
Yes, 4 backslashes for a single backslash, because Both Json and Properties use Adding other ascii aliases seems a bit stupid (but we had to, I'd pick all unicode names and aliases or all ascii names in unix
However, aliases for controls characters is an idea, |
| put("PAGE-DOWN", "PGDN"); | ||
|
|
||
| put("DELETE", "DEL"); | ||
| put("BACKSPACE", "BKSP"); |
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.
put("BACKSLASH", "\\");
put("QUOTE", "\"");
put("APOSTROPHE", "'");|
@Grimler91 In your comment below the screenshot, write that you wrote |
|
@fornwall this PR is ready to be merged in! A screenshot explains the thing when Release notes include:
|
|
Is it possible to use |
|
@tomty89 what's the benefit of that? Feels like it would make the file much harder to find. |
|
It's not really much harder, but yeah it might feel so (and out-of-comfort-zone). Well there are couple reasons (which might be a bit personal and may or may not be valid):
|
|
The app currently uses SharedPreference interface for some settings (I've seen it somewhere in the source code), I don't know for what. But it is but it seems to be a classic way to store settings in android. We could add that in another small PR, the idea would be also to have a classic "settings" menu when some useful settings will be changed in a gui, the dotfile would override. We currently look for multiple locations and that's a good thing. About xml, we could also add a simple xml interface when we'll add JSON (currently only java.util.Properties type is allowed), if people likes xml more than JSON or properties, why not. However, xml is verbose, JSON less, properties is a good tradeoff. On Ubuntu, that's standard to keep app infos in ~/.config, because the $HOME dir is related to the user whereas /etc is about root. When people back their stuff, they backup $HOME. About the directory /data/data/com.termux/, if it's standard on Android app (I'm no Android expert), we'll add the location. About the filename, com.termux_preferences, is that standard? Or just your first idea? |
|
The file is created by the SharedPreference interface. I'm sure you have it in your Termux as well. That's a reason I think it is preferred, as it seems to be the orthodox one where you don't even need to deal with the path yourself. What I consider bad is that Termux has been making use of two different interfaces ( Semantically speaking I don't think we should put app settings (i.e. settings that has nothing to do with programs we uses in Termux or terminal emulation) anywhere under |
|
Ok I see, the file in The current advantage of And again, the advantage of java.util.Properties is that it's very brief and not verbose. But I agree this does the trick too: <?xml version='1.0' ?>
<map>
<string name="extra-keys">[
["CTRL", "ALT", "DELETE"]
]</string>
</map>Xml does ask more questions, <!-- very verbose -->
<?xml version='1.0' ?>
<map>
<rows name="extra-keys">
<row>
<key>CTRL</key>
<key>ALT</key>
<key>DELETE</key>
</row>
</rows>
</map>I prefer the What is the point of view of @fornwall ? Why did you choose ~/.config and java.util.Properties instead of SharedPreferences for the keys? However I think this can be in a future PR, and using something standard for android is nice. As the dev guide says, SharePreferences is useful for storing simple key/value data, which is what our |
|
However, I think the new default must be reconsidered:
I suggest the new default being: I don't think there is enough space to have both "- / |" and "↓↑". The choice of "-" is kind of arbitrary, but I think "-" is more useful, to provide options and termux-like-commands. Also, the order ESC TAB CTRL ALT seems better too, because ESC is expected to be to the left-most (useful in vim), TAB also, and about CTRL and ALT, they are always followed by another letter. And of course, we'll find later a way to explain to people how to write the |
|
@fornwall any feedback/approbation? |
|
@robertvandeneynde Thanks a lot, and sorry for the delay! I'll try it out during the coming couple of days and make a release shortly. |
|
I figured out you were on vacation :) I'm still wondering if the default should be on one line with arrow keys like in my last commit or on two lines, and if show_extra_keys should be displayed by default (I think yes, majority of people don't know about changing keyboards or can't plug a real keyboard). And also, because the left menu cannot be found easily, add an entry "show side menu" to the long press menu. And a text "long press on the screen to see the options" in the welcoming message in the terminal. For the future, I think some accessibility are useful, I'm seeing a classic "settings" (Preferences) panel. Such that basic settings can be changed in a classic way, without editing dot files. |
|
@fornwall ? It's been 2 months when you said "soon" :p Isn't there someone else that can test and publish a new version on the store ? |
Here's an rebased and updated version of @neverwin's configurable keys (#650). Ping @robertvandeneynde since you had good input in previous PR.
Some stuff remains: I haven't managed to get the property parser to accept arrow keys in termux.properties, if I for example tryextra-keys = [['ESC','/','―','HOME','↑','DEL','-'],['TAB','CTRL','ALT','←','↓','→','PGDN']]I get:Which I suppose is a UTF-8/UTF-16 error, and should be fixable..
Writing
extra-keys = [['ESC','/','\u2015','HOME','\u2191','DEL','-'],['TAB','CTRL','ALT','\u2190','\u2193','\u2192','PGDN']]instead works:I also think that the keys take up too much vertical space, making the rows smaller would be nice. @xqdoo00o I suppose that this is what you are doing in #759? But that works only for android 5? I didn't notice any big difference when I merged it (https://github.com/Grimler91/termux-app/tree/configurable_keys_smaller)
Also, more importantly, we/I need to add more validation of the
extrakeysfield! If, for example, two rows of different lengths are given the app crashes on start-up (with no change of fail-safe session).I've also set the default extra keys to default to the single row we had before, and made the extra keys visible per default. Users who want several rows can configure it.
We should definitely ship a default termux.properties file with all the different options as comments, they are a bit secret as mentioned in the previous PR.
I've also kept the "swipe up from - for |" and "swipe up from / for " features, though they are less needed with configurable keys.