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

Feature: Polls & Poll Answers #3248

Closed
wants to merge 17 commits into from
Closed

Conversation

keaysma
Copy link

@keaysma keaysma commented Dec 15, 2024

Recently I read #1310 and got interested in what an implementation of polls could look like.

This is partially complete, the main work done here is defining lexicons for polls as embeds of posts, as well as pollAnswers, which are their own records which copy a lot of stuff from likes. Polls contain a set of strings which are possible answers an actor could pick, and pollAnswers container an integer which is intended to reference a selected answer chosen by an actor. Importantly, the answer field of pollAnswer is intended to be used as a bitmask. This way, polls could support multi-answer selections in the future with little additional work.

This implementation is WIP, and experimental I have not finished writing up the aggregation code for poll answers, and, I'm sure I'm missing more things. I'd like to get an early sense of how people feel about this implementation! I could also use some mentoring on working with the atproto codebase. I have some of it down, but, I'm sure i'm missing things, and, I don't know what I don't know.

Largely, this is a 'playground' PR for determining if the lexicon is going to make sense for implementation.

I have opened a separate, non-draft PR, where I will collect feedback on the lexicon only: #3298

"defs": {
"main": {
"type": "query",
"description": "Get poll answers for a given poll which reference a post.",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"description": "Get poll answers for a given poll which reference a post.",
"description": "Get poll answers which reference a given post.",

I think this is a bit confusing in terms of protocol design, since for example the poll could be edited out of a post and nothing is stopping a user from submitting a app.bsky.feed.pollAnswer that references a post that doesn't exist (yet) or doesn't have a poll (yet). The latter case could easily happen during normal use if multiple AppViews are involved.

Copy link
Author

Choose a reason for hiding this comment

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

That's a downside to the implementation I have put together so far. I considered instead creating an individual record called a Poll, which would then be associated with a Post, which could mitigate this. The issue I had with that is then you have this object that can only really ever have a relationship to a post, or can otherwise just be orphaned, and I figured that wouldn't be very useful.

I'm open to changing this though!

},
"minLength": 2,
"maxLength": 4
}
Copy link

@Tamschi Tamschi Dec 16, 2024

Choose a reason for hiding this comment

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

This only covers single-choice polls in terms of poll definition.

Rather than a binary single-choice and any-number-of-choices system, it would be cool to have the maximum number of distinct selections given as an integer:

Suggested change
}
},
"maxAnswers": {
"type": "integer",
"minimum": 1,
"default": 1
}

It should be specified what happens when the number is changed, but that's necessary anyway because multiple-choice polls (if/once allowed) can be edited into single-choice ones and vice versa. (The easiest solution would be to discard/disregard all previous answers when poll post content is edited of course, which is what Mastodon does I believe.)

Copy link
Author

Choose a reason for hiding this comment

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

This is good to have too. I'm going to hold off on committing this in for now, as I would like to figure out the validation behavior that will allow this to work properly. Your mention of poll editing is key here. In the last question that I answered I was considering moving to having Polls be their own record, and, that may be necessary to handle poll editing properly, ensuring that PollAnswers are discarded, by nature of referencing a stale copy of the poll at edit time (that is to say, editing a poll would create a new poll record).

@Tamschi
Copy link

Tamschi commented Dec 16, 2024

Aside from the review comments, the option limit of 4 seems quite low.
Mastodon allows 12 and subjectively I haven't seen that abused yet.

(The eventual UI may need some mitigation for page stretching for the overall post, however, as even (1+4) times 300 graphemes is quite long in this context. Mastodon has a shorter limit here, it allows (entry of! It likely can display many more.) up to 50 characters per answer.)

@Tamschi
Copy link

Tamschi commented Dec 16, 2024

I feel like app.bsky.feed.getPollAnswers should have a way to also filter answers to those selecting a specific option only.

That would give a bit more freedom on how the UI for browsing those should work, i.e. for paginating through specific selections individually when they are listed separately.

keaysma and others added 3 commits December 16, 2024 20:56
Co-authored-by: Tamme Schichler <tamme@schichler.dev>
Co-authored-by: Tamme Schichler <tamme@schichler.dev>
Co-authored-by: Tamme Schichler <tamme@schichler.dev>
@keaysma
Copy link
Author

keaysma commented Dec 17, 2024

Aside from the review comments, the option limit of 4 seems quite low. Mastodon allows 12 and subjectively I haven't seen that abused yet.

(The eventual UI may need some mitigation for page stretching for the overall post, however, as even (1+4) times 300 graphemes is quite long in this context. Mastodon has a shorter limit here, it allows (entry of! It likely can display many more.) up to 50 characters per answer.)

Thank you for the info, the setup I have here is configured pretty differently. While I set the initial protocol-level maximum to 4, I chose a database type of int8, which should allow for 8 bit-masked answers. That's still less than 12 of course, so, perhaps it'd be better to use just a UNSIGNED BIG INT to set up flexibility. This does come at the cost of requiring more space to store poll answers however, regardless of how many answers people use in practice.

@Tamschi
Copy link

Tamschi commented Dec 17, 2024

I think it's fine to limit it to a value that can be stored in an integer bitfield.
That should allow at least 32 poll options if I had to guess, which seems like quite a bit more than reasonable.

@keaysma keaysma marked this pull request as draft December 23, 2024 17:39
@keaysma
Copy link
Author

keaysma commented Dec 28, 2024

I got very ahead of myself in this branch and did a bunch of work/codegen. Most of this was just because I was curious to see how implementing the lexicon would look. That being said, that's against the contribution guidelines, so, I will be closing this PR, keeping it only for the sake of reference.

I'd like to move any discussions or comments for feedback to a new PR: #3298

This is based on a separate branch, where I have cherry-picked all of the commits from this PR's branch which pertain to lexicon changes only.

@keaysma keaysma closed this Dec 28, 2024
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.

None yet

2 participants