Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add post editing capability #2828

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Add post editing capability #2828

merged 7 commits into from
Dec 8, 2022

Conversation

Tak
Copy link
Collaborator

@Tak Tak commented Nov 14, 2022

This doesn't seem to require a ton of additional work from delete/redraft.

Verified use cases:

  • Edit post, send
  • Edit post, send fails, resend from draft
  • Edit post, save draft, resend from draft

We still don't have a solution for support detection (status.edited_at will be null both when the instance doesn't provide it and when the instance sends it as null (thanks, json!))

I'm also not sure what to do with the edited date: set it as the date for the post in the timeline? ignore it? something else? (covered by #2935)

Finally, I haven't thoroughly tested all the edge cases yet. I feel like the fact that we're leveraging the existing draft system etc. means that it should be pretty solid, but there are still a lot of different variables wrt attachments, polls, scheduling, etc., that need to be actually explored.

#2759

@Tak
Copy link
Collaborator Author

Tak commented Nov 21, 2022

Found two TODOs just now:

  • It looks like we have to always reupload media like we do with delete and redraft
  • We also need to fix up mentions (and presumably hashtags)

@sl1txdvd
Copy link

sl1txdvd commented Nov 21, 2022

Not a dev so apologies in advance if I state obvious things or ask stupid questions. Not sure if I am supposed to answer here as a user.

We still don't have a solution for support detection

Checking the Mastodon version per account (maybe at login) would give you that information and might be useful for future features too. Probably outside of the scope of this feature.

I'm also not sure what to do with the edited date: set it as the date for the post in the timeline? ignore it? something else?

The edited post should stay at the original place/time in the timeline. But there needs to be a way to open the revisions. In the web client there is an arrow or something next to the date/time when you open an edited post. There you can open the revisions.

there are still a lot of different variables wrt attachments, polls, scheduling, etc.,

Polls can be edited as a whole and restarted, but it is not possible to edit the options of a running poll.

@connyduck
Copy link
Collaborator

I think assuming the server supports it is fine. Most Mastodon instances are on 4.0.

It looks like we have to always reupload media like we do with delete and redraft

Wait, with delete & redraft we don't reupload but reuse the media id, this should work here as well 🤔

@connyduck
Copy link
Collaborator

connyduck commented Dec 5, 2022

We also need to fix up mentions (and presumably hashtags)

There is an api where one can get the post source: GET /api/v1/statuses/:id/source

@Tak
Copy link
Collaborator Author

Tak commented Dec 5, 2022

Ok, mentions actually do work - the problem I saw earlier was either workflow-related, or maybe because I was testing while all instances were heavily loaded with new users

@@ -138,6 +138,7 @@ class SendStatusService : Service(), Injectable {
statusToSend.mediaProcessed.forEachIndexed { index, processed ->
if (!processed) {
when (mastodonApi.getMedia(statusToSend.mediaIds[index]).code()) {
404, // This can happen when editing posts with media that were uploaded a long time ago
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@connyduck Does this make any sense to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, because that codepath should only be hit when reuploading 🤔

@Tak
Copy link
Collaborator Author

Tak commented Dec 6, 2022

Ok, I think this is ready for other people to try it.
Tested scenarios:

  • Edit the text of a post (+/- media)
  • Edit the text of a reply (+/- media)
  • Add/remove content warning on a post (+/- media)
  • Add/remove media from a post
  • Change media sensitivity on a post
  • Change language of a post

@Tak Tak marked this pull request as ready for review December 6, 2022 14:00
@Tak Tak changed the title [WIP] Add post editing capability Add post editing capability Dec 6, 2022
Copy link
Collaborator

@connyduck connyduck left a comment

Choose a reason for hiding this comment

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

Works like a charm! I wonder if we should somehow indicate that one is currently in editing mode?

@@ -137,7 +137,7 @@ data class Status(
)
}

private fun getEditableText(): String {
fun getEditableText(): String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, don't use this. This is only here to have a fallback for ancient Mastodon versions before deleting returned the status source. To get the status source without deleting a status, call GET /api/v1/statuses/:id/source.

Copy link
Collaborator

@connyduck connyduck left a comment

Choose a reason for hiding this comment

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

nice!

abstract class SFragment : Fragment(), Injectable {
protected abstract fun removeItem(position: Int)
protected abstract fun onReblog(reblog: Boolean, position: Int)
private var bottomSheetActivity: BottomSheetActivity? = null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private var bottomSheetActivity: BottomSheetActivity? = null
private lateinit var bottomSheetActivity: BottomSheetActivity

then you don't need any calls with ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, thought I had caught all of these

if (activeAccount != null) {
loggedInUsername = activeAccount.username
}
val mentionedUsernames: MutableSet<String?> = LinkedHashSet(
Copy link
Collaborator

Choose a reason for hiding this comment

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

MutableSet<String>, then you don't need the map { it!! } below

menu.add(0, R.id.pin, 1, getString(if (status.isPinned()) R.string.unpin_action else R.string.pin_action))
}
Status.Visibility.PRIVATE -> {
var reblogged = status.reblog?.reblogged ?: status.reblogged
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var reblogged = status.reblog?.reblogged ?: status.reblogged
val reblogged = status.reblog?.reblogged ?: status.reblogged

menu.findItem(R.id.status_unreblog_private).isVisible = reblogged
}
else -> {
// TODO: anything?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so 🤔

AlertDialog.Builder(requireActivity())
.setMessage(R.string.dialog_redraft_post_warning)
.setPositiveButton(android.R.string.ok) { _: DialogInterface?, _: Int ->
timelineCases!!.delete(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
timelineCases!!.delete(id)
timelineCases.delete(id)

.show()
}

private fun editStatus(id: String, position: Int, status: Status) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

position is unused

startActivity(newHashtagIntent(requireContext(), tag))
}

protected fun openReportPage(accountId: String, accountUsername: String, statusId: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be private (showConfirmDeleteDialog as well)

@connyduck connyduck merged commit a6b6a40 into tuskyapp:develop Dec 8, 2022
@connyduck connyduck mentioned this pull request Dec 8, 2022
1 task
@connyduck connyduck mentioned this pull request Dec 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants