Skip to content
Snippets Groups Projects

qt: support non-standard mouse and trackpad wheel events in player view

Closed Fatih Uzunoğlu requested to merge fuzun/vlc:qt/supportnonstandardmousewheel into master
1 unresolved thread

This makes it possible to use trackpad in order to adjust the volume and time in player view.

Note that this does not bring support for high-precision/fine adjustment. That is handled in !5391.

Edited by Fatih Uzunoğlu

Merge request reports

Merge request pipeline #489814 passed

Merge request pipeline passed for 1dbe0145

Test coverage 18.36% (0.05%) from 1 job
Approval is optional

Closed by Fatih UzunoğluFatih Uzunoğlu 4 months ago (Dec 21, 2024 5:57pm UTC)

Merge details

  • The changes were not merged into master.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
298 298
299 299 int qtWheelEventToVLCKey( const QWheelEvent& e )
300 300 {
301 static QPoint p;
  • this looks like a bad idea to have a static variable here, this means that if you have multiple widgets using this function, when you start scrolling in a different item you'll end up with the leftover from the previous one.

    I think we should:

    • make this an object that could be attached to the surface
    • feed the wheel events
    • when you have enough delta for a step emit a signal to notify the scroll steps ( This would not limit the function to a single step per wheel event)
    • reset the scroll leftover when the direction changes
  • Author Reporter

    Same matter when the user switches mouses (for example, using trackpad and mouse). I was aware of the consequences for using static variable here.

    In my opinion, while not ideal, it is acceptable to behave like that. I think we should have a separate merge request in the future if we want to go the way you propose. At least currently, this fixes the issue of not being able to use trackpad scroll.

    Having the leftover in another widget would only affect trackpad (not standard mouse), and that is only for the initial action. That is probably not nit-picking, but more close to being "nit" than a real issue.

  • Author Reporter

    Follow up @chub? I really prefer this to be merged (considering it is really simple and does not cause maintenance burden and can serve as a foundation for !5782 (merged)), even if !5782 (merged) is going to be merged in the future.

  • Author Reporter

    Note that it is currently completely unusable without this change, "end up with the leftover from the previous one" is a significantly less concerning issue, so I don't really get the objection. We have been doing iterative development, if there is a better way to do that, then in the future it can be merged, it does not mean this should be blocked (for months).

  • I think !5782 (merged) is the right solution for this. Do you have follow up on that MR?

  • Author Reporter

    I think !5782 (merged) is the right solution for this. Do you have follow up on that MR?

    My latest reply (private) was to proceed with it, because I have not had anything better to offer. I proposed to have event filter and adjust the event, but events don't seem to be possible to be adjusted (I realized that after the discussion). Please also check yourself if it can be done in a more elegant way.

  • Please register or sign in to reply
  • added MRStatus::InReview label and removed MRStatus::Reviewable label

  • Author Reporter

    @chub I think I can not handle compressed events or weird mouses here (delta greater than 120). This function can only report a single key. Either the caller needs to handle that (then, it is not relevant to this merge request), or I need to return a list of keys here (I think bad idea).

    In fact, if this is a problem, it is already a problem.

  • Pierre Lamot mentioned in merge request !5782 (merged)

    mentioned in merge request !5782 (merged)

  • **** added MRStatus::Stale label and removed MRStatus::InReview label

    added MRStatus::Stale label and removed MRStatus::InReview label

  • added MRStatus::InReview label and removed MRStatus::Stale label

  • Author Reporter

    Closing, since !5782 (merged) is merged now as a substitute.

  • Please register or sign in to reply
    Loading