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

Implement roles for volume buttons: do nothing, change text size, navigate sent history #554

Merged
merged 1 commit into from
Nov 26, 2022

Conversation

zopieux
Copy link
Collaborator

@zopieux zopieux commented Nov 26, 2022

The navigation state is saved and restored along with BufferFragment's lifecycle.
The deprecated preference is migrated to the correspondig volume role.

Supersedes #297. Fixes #553.

…igate sent history

The navigation state is saved and restored along with BufferFragment's
lifecycle.

The deprecated preference is migrated to the correspondig volume role.
Comment on lines +702 to +704
<string name="pref_buttons_volume__actions__none">Ничего не делать</string>
<string name="pref_buttons_volume__actions__text_size">Кнопки громкости меняют размер текста</string>
<string name="pref_buttons_volume__actions__history">Навигация по истории отправлений</string>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Machine-translated, probably wrong.

Comment on lines +706 to +708
<string name="pref_buttons_volume__actions__none">Nichts tun</string>
<string name="pref_buttons_volume__actions__text_size">Lautstärketasten ändern die Textgröße</string>
<string name="pref_buttons_volume__actions__history">Navigieren im Sendeverlauf</string>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Machine-translated, probably wrong.

@zopieux zopieux requested a review from mhoran November 26, 2022 20:50
@mhoran mhoran merged commit 8e35a66 into master Nov 26, 2022
@zopieux zopieux deleted the volume-btn-roles-new branch November 26, 2022 21:02
Copy link
Collaborator

@oakkitten oakkitten left a comment

Choose a reason for hiding this comment

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

The biggest problem here is behavior.

  • 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.

  • More importantly, when pressing up, current input is preserved. But when you navigate up AND edit a previous message AND navigate elsewhere, that last input is NOT preserved. 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.

  • When switching between messages the cursor flickers to start of the string and to end, this can probably be avoided?

Finally, in #553 the user asked about history navigation with Up/Down arrow keys, NOT volume buttons, and this PR doesn't fix that so it shouldn't have been closed

@@ -64,15 +64,8 @@ import com.ubergeek42.WeechatAndroid.upload.i
import com.ubergeek42.WeechatAndroid.upload.insertAddingSpacesAsNeeded
import com.ubergeek42.WeechatAndroid.upload.suppress
import com.ubergeek42.WeechatAndroid.upload.validateUploadConfig
import com.ubergeek42.WeechatAndroid.utils.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: not a fan of wildcard imports, would rather prefer smaller diff

app/src/main/res/values/strings.xml Show resolved Hide resolved
app/src/main/res/values/strings.xml Show resolved Hide resolved
@zopieux zopieux restored the volume-btn-roles-new branch November 27, 2022 00:42
@zopieux
Copy link
Collaborator Author

zopieux commented Nov 27, 2022

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 switching between messages the cursor flickers to start of the string and to end, this can probably be avoided?

Resolved by removing post.

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. At which point do you forget these edits of past sent messages? Do you add a new version in-between that's preserved in sentMessages somehow?

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.

Ah nice catch. That's because I don't reset the index when sending.

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.

Up Arrow to last command
3 participants