Volume buttons: implement Weechat-like input history rather than sent history

[zopieux]

Please see #554 comments.

• Similar behavior to Weechat native input
• Don't store whitespace-only messages
• Prevent navigation if user has non-uploaded ShareSpans, and display an error toast

[zopieux]

Index is saved per-buffer, but we are using a global array of sent messages. Sending messages in another buffer will make index in current one invalid.

I don't think this is a real issue since we don't have per-buffer history. What do you suggest instead?

when you navigate up AND edit a previous message AND navigate elsewhere, that last input is NOT preserved

That's correct. I don't think I want to fix that, that would basically mean introducing some kind of forking history. I don't want to persist changes to sentMessages (obviously) and I'm not comfortable having an app-wide fork/editable verrsion of sentMessages. Suggestions welcome though.

Moreover, if you send it, the NEXT input that you type in is NOT preserved when navigating away. This is not an edge case and should be addressed.

Fixed. When sending, if user was then navigating the history, I restore the previous input.

[oakkitten]

By the way this can be taken as we make volume buttons do nothing at all, perhaps we could say “Change volume” or some sort of a “bypass” or “no special action” variant? Maybe

[oakkitten]

This is a constant so should be all caps

[oakkitten]

I think this would be easier to read

val newIndex = when (direction) {
    Direction.Older -> sentMessageIndex + 1
    Direction.Newer -> sentMessageIndex - 1
}

[zopieux]

Fixed.

[oakkitten]

Again, what is value of name?... What change_text_size is to ChangeTextSize is ”a string representation of enum value” (for the purpose of storage). (CMIIW: VolumeRole is “an enum” and ChangeTextSize is “an enum value”.) I'd use stringRepresentation, or just string for shirt and because it pairs nicely with factory methods from*.

And this is a kind of a factory methods and I prefer naming them all from*, here it would be fromStringRepresentation() or fromStoredString() or fromPersistedString() or just fromString(), like here:

weechat-android/relay/src/main/java/com/ubergeek42/weechat/relay/connection/Handshake.kt

Lines 121 to 137 in 822ceb7

	private enum class Algorithm(
	val string: String,
	val shaSize: Int,
	val fast: Boolean,
	val canHandleEmptyPassword: Boolean,
	) {
	Plain("plain", 0, true, true),
	Sha256("sha256", 256, true, true),
	Sha512("sha512", 512, true, true),
	Pbkdf2Sha256("pbkdf2+sha256", 256, false, false),
	Pbkdf2Sha512("pbkdf2+sha512", 512, false, false);
	companion object {
	fun fromString(string: String) = Algorithm::string.find(string)
	?: throw IllegalArgumentException("Unsupported password hash algorithm: '$string'")
	}
	}

[zopieux]

Fixed.

[oakkitten]

First of all let's see how WeeChat does it. We are not keeping per-buffer history and I think doing that would be overkill, and thus we can't follow WeeChat's algorithm exactly, but it should be a good reference point.

• When you type input and press Down, current input is pushed into history and cleared.
• When you navigate up the history and type, and then
  • navigate away, that history entry is edited in place;
  • send it, a new entry is pushed into history, and the historic entry is not edited in place, and input is cleared.

    • IF before navigating history user had some input, this input appears in history above the last sent message. To illustrate,

      H                       H sent message 1
      H                       H sent message 2
      H sent message 1        H current input
      H sent message 2   ->   H sent message 2 edited
      > current input         >
      

I don't think this is a real issue since we don't have per-buffer history. What do you suggest instead?

I think we can keep buffer indexes valid. That is, if you have foo bar baz in history and you are looking at bar in input, pressing Up should always take you to foo. Our sent messages are essentially an equality-based set, so I think we can do this by simply storing the sent message itself instead of the index, or maybe store some kind of a wrapper with an ID attached.

That's correct. I don't think I want to fix that, that would basically mean introducing some kind of forking history. I don't want to persist changes to sentMessages (obviously) and I'm not comfortable having an app-wide fork/editable verrsion of sentMessages. Suggestions welcome though.

Forked history is awful but we could:

• Allow editing global history, similar to WeeChat. You'd need stable IDs for messages then to implement ↑
• Do a hack, maybe? Like, if the change is significant, say, more than 5 or 10 characters, push the edit into history. I don't like this much since if you press ↑ and then ↓ (or ↓ and ↑, or both) you won't be getting what you have started out with, but at least the input will not be lost.
• Ignore user edits of history. Sucks to lose input, but this will occur very rarely.

Fixed. When sending, if user was then navigating the history, I restore the previous input.

It's a bit weird to have your previous input pop up like that. I'd prefer the WeeChat way, pushing user input into history instead of restoring it.

[oakkitten]

Some thoughts. Placing current input into history is going to be a bit problematic. Current input can currently have images, but sent messages are text-only objects. The Paste dialog should not have issues with this, but:

• We save/restore sent messages using serialization and it probably doesn't support serializing spans, even if we make all our spans serializable. Same probably goes for bundles?
• Even if we find a way to restore spans, can we be sure that we'll have the necessary URI permissions or thumbnails?
• Suppose you send a message with images, after converting images to text. In history that'd be different messages, but they probably should be one.

[mhoran]

I don't think this has to be perfect. We can get it mostly right and iterate from there. I don't think we need to restore image previews/thumbnails. If I've posted an image to a previous channel and want to repost to another, I can use the up arrow and there will be the URL to that image, for example.

[oakkitten]

Even if we aren't putting thumbnails into history, we have to do something with them don't we

[mhoran]

I'm confused. When I send an image in Weechat-Android it changes the image to a URL. Then I go to another buffer with this feature enabled and I hit the up arrow and get the URL. It works as expected. So I don't understand what needs to be done here?

There are no image previews in my history so far as I can see.

[oakkitten]

Yes, but if we are following the WeeChat way we will also want to put unsent messages to history, and these can contain images. Like if you have an image in current input and you press down. (Or up)

[mhoran]

Let's just not do that for now? It sounds complicated, and is not really how I would be using this feature.

[zopieux]

I think I previously missed the fact @oakkitten and I were just not on the same page as to what this feature is about. @oakkitten wants to replicate Weechat native local input edit history (including stuff like pushing unsent messages when pushing ↓) whereas from the get go, my goal was simply to make the sent messages (the modal that appears with a long-tap) easily accessible using volume keys. That difference in intended goals explains a lot of the comments.

Now that I've laid that down, I suppose we can actually bite the bullet and implement a proper Weechat-like input history just like @oakkitten wants. Here is what this would look like, I believe I've addressed all edge-cases (while ignoring image uploads):

• pressing down from nothing saves in history then clears input
• pressing up from nothing saves in history (if not empty) then navigates
• pressing up/down navigates the history
• modifying a historical message then pressing up/down without sending saves the edited message in-place then navigates
• modifying a historical message then sending it keeps the historical message intact and saves the modified message as the most recent historical message
• deduplication works just like Weechat: message is saved only if it's different from the last historical one
• when reaching either end (oldest message or cleared), navigating further is a no-op i.e. we don't re-update the input with the same content
• I forgot to show it in the screencast, but I'm also preserving the user selection when sending / editing historical messages; it's a nice-to-have, but I think it actually is useful. This means that by default (no selection), the cursor moves to the end, which is what the user expects anyway.

Making this history app-global versus buffer-local is a trivial change (if global I would store it in P, just like sentMessages), so please opine. I believe @mhoran would prefer a global history.

If you're fine with the described behavior, I'll update this PR for review.

weechatvolume.mp4

[oakkitten]

Haven't looked at the code yet, but here's my thoughts from running the code

• Blank messages (whitespace-only) can make it into history. Not sure if previous code allowed it? Best to disallow because it looks weird.

• Have image in input, press Up, Down, and instead of the image you see a space. I don't think losing input here is acceptable. Either allow putting images into to history, or disallow navigating away from input that contains images. Maybe with an error toast?

• Got this crash while (I think) pressing Up in a different buffer

  java.lang.ArrayIndexOutOfBoundsException: length=10; index=-1
    at java.util.ArrayList.get(ArrayList.java:439)
    at com.ubergeek42.WeechatAndroid.utils.History.navigate(History.kt:82)
    at com.ubergeek42.WeechatAndroid.fragments.BufferFragment.uiInputHardwareKeyPressListener$lambda-34(BufferFragment.kt:527)
    at com.ubergeek42.WeechatAndroid.fragments.BufferFragment.$r8$lambda$4ELeM4kXfS7-o8595LjxnDqbenw(Unknown Source:0)
    at com.ubergeek42.WeechatAndroid.fragments.BufferFragment$$ExternalSyntheticLambda13.onKey(Unknown Source:2)
  

• When buffer A is showing a historic message, and in buffer B you send a message, buffer A's input is then treated like regular input, that is pressing Down in buffer A, instead of navigating to the newer entry, pushes input into history.

• Buffer histories seem to be able to contain duplicate messages, while paste dialog doesn't do it. Probably better to disallow duplicate histories?

[zopieux]

Buffer histories seem to be able to contain duplicate messages, while paste dialog doesn't do it. Probably better to disallow duplicate histories?

This is Weechat behavior. If you insert foo, then bar, then foo, foo is there twice. If you want consistency with Weechat, let's not change that.

Blank messages (whitespace-only) can make it into history. Not sure if previous code allowed it? Best to disallow because it looks weird.

Sure let's do that.

disallow navigating away from input that contains images

That sounds annoying but OK I guess. How do I detect there is an image?

[oakkitten]

If you want consistency with Weechat, let's not change that.

More than that I want to have consistency within the app itself. 😛 And it would simplify things no? Right now you need to maintain several lists

That sounds annoying but OK I guess. How do I detect there is an image?

Probably less annoying than having them images replaced with spaces. You can use hasShareSpans() I think?

[zopieux]

Got this crash

I think fixed it. The updated code can't reach index == -1 anymore.

Probably less annoying than having them images replaced with spaces. You can use hasShareSpans() I think?
We save/restore sent messages using serialization and it probably doesn't support serializing spans, even if we make all our spans serializable. Same probably goes for bundles?

OK so it seems like I can make this work with ShareSpan, but neither Suri nor Bitmap are Serializable, and they need to be since the History is saved state. Bitmap is especially tricky because even if it's Parceable and therefore marshallable, it has Binders so marshall fails. I could export the bitmap to some format that can be serialized and unserialized, but wow that's a lot of code for something that, let's be honest, won't ever be used (saving a not-yet-uploaded bitmap in the input history).

What do you prefer?

1. status quo, these spans are ignored and replaced by either spaces like now, or some placeholder "[image]" text
2. disable navigation in the history as long as the user has a non-uploaded file ← I went with that in the latest version
3. go forward with making ShareSpan serializable, which involves making ShareSpan parceable & marshall to bytes, which involves rasterizing the Bitmap
4. don't try to serialize (save) the History at all; it won't resist Android recycling the app, which I think is a shame for a "history" feature

[mhoran]

This is working pretty well for me. If there are further changes we'd like to make we could do those as a follow up. Any objections to merging?

[zopieux]

I mean this PR is definitely better (functionally wise) compared to master. @oakkitten might have code nits we can address when they're back online. OK to merge as-is.