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

Close hide filename in upper left (suppress OSD), #4197 #4991

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

low-batt
Copy link
Contributor

This commit will:

  • Add the settings disableOSDFileStartMsg, disableOSDPauseResumeMsgs, disableOSDSeekMsg and disableOSDSpeedMsg
  • Add a new isDisabled property to OSDMessage
  • Change PlayerCore.sendOSD to not display the OSD message if it has been disabled
  • Add a Suppress message for disclosure triangle in the On Screen Display section of the UI tab that holds checkboxes for the OSD messages that can be disabled

These changes add settings that allow the user to keep the OSD enabled while suppressing some of the OSD messages. This is useful in certain applications such as looping in a kiosk or scrubbing through a video without distractions.


Description:

This commit will:
- Add the settings disableOSDFileStartMsg, disableOSDPauseResumeMsgs,
  disableOSDSeekMsg and disableOSDSpeedMsg
- Add a new isDisabled property to OSDMessage
- Change PlayerCore.sendOSD to not display the OSD message if it has
  been disabled
- Add a "Suppress message for" disclosure triangle in the On Screen
  Display section of the UI tab that holds checkboxes for the OSD
  messages that can be disabled

These changes add settings that allow the user to keep the OSD enabled
while suppressing some of the OSD messages. This is useful in certain
applications such as looping in a kiosk or scrubbing through a video
without distractions.
@low-batt low-batt requested a review from uiryuu June 11, 2024 19:31
@low-batt low-batt linked an issue Jun 11, 2024 that may be closed by this pull request
@low-batt
Copy link
Contributor Author

This is what this portion of settings looks like with the changes:
suppress-osd

@svobs
Copy link
Contributor

svobs commented Jun 12, 2024

Nice idea. Can I suggest inverting the state so each checkbox represents an "enable" instead of a "suppress"? I believe there is a school of thought that toggle controls are more intuitive when there their "on" state matches the "on" state of the action they represent (and/or that reducing negations improves ease of understanding). I was hoping to find this mentioned in Apple's HIG but they are unfortunately silent on it.

@low-batt
Copy link
Contributor Author

I agree that having settings that enable messages would be more intuitive. I did not do that because that would require more work now and require continuing work as new OSD messages are added. Although restrictive, the "suppress" design limits the work and still addresses the problem posed by the issue. I envisioned an "enable" approach as a part of a much more extensive feature set that allowed additional OSD configuration abilities similar to what can be done with mpv's OSD configuration.

There is another change that is needed to address the issue. We need to add an Enable OSC setting. I remember you working on this? Or maybe we just discussed this? I remember concerns about the OSC being tied to the title being shown/hidden. My thoughts on that is that it would be fine to initially tie that to the OSC setting. A user should be able setup IINA to loop a playlist and not have the OSC or OSD appear. With the changes in this PR the user can keep the OSD from appearing without having to fully disable it. But the OSC appears when a file starts.

Another reason to add an Enable OSC setting is that we have just added the opposite of that. You can now keep the OSC visible at all times. Would be good to add this to complete the ability to control the OSC.

Do you want to take on adding Enable OSC?

@uiryuu
Copy link
Member

uiryuu commented Jun 12, 2024

I think it would be better to let users to select what they message they want to show for all message types. We can make a dedicate page for this and start a sheet window when the user click on a "Configure OSD" button for example. Although I did't try to accomplish this nor know the code complexity in order to achieve this feature.

@low-batt
Copy link
Contributor Author

I was hoping to get this PR merged first before fixing another OSD defect that makes it clear it can not be all OSD messages. We have a few OSD "messages" that must not be suppressed by Enable OSD. Right now that suppresses the UI for downloading subtitles. A user who uses the mpv OSD instead of IINA's thought the downloading subtitles feature was just totally broken since "nothing happened" when the menu was clicked. I was planning on adding a isAlwaysShown property to OSDMessage to cover that case.

A solution that exposes all (excluding the mandatory ones) OSD messages really should be data driven, so adding a new enum to OSDMessage automatically appears in the UI. Very doable, but a lot more work. This also means the enum can't have "generic" messages that actually represent multiple different kinds of messages like the current "custom" entries do.

If we are doing significant work on OSD messages it really should including features like mpv has. With the plugin system we should be able to support parameterized expressions like mpv does. Of course this is even more work.

At the moment we have a lot of other more important changes that need to be made. So this proposal is all about covering the most common case with minimum work expecting it will be replaced by a significant enhancement to OSD abilities in the future.

@low-batt
Copy link
Contributor Author

I went ahead and posted PR #4992 which is about the problem with disabling the OSD breaking the Find Online Subtitles feature. That shows the issue with allowing the user full control over all of the OSD messages. They may not understand that these are not all optional notifications and disabling some of them will break a feature.

@svobs
Copy link
Contributor

svobs commented Jun 15, 2024

I agree that having settings that enable messages would be more intuitive. I did not do that because that would require more work now and require continuing work as new OSD messages are added. Although restrictive, the "suppress" design limits the work and still addresses the problem posed by the issue.

Not sure I see this reasoning. The "default" for things without checkboxes isn't tied to anything. And in fact, an individual checkbox can be the inverse of its stored pref value binding by using the value transformer NSNegateBoolen.

I envisioned an "enable" approach as a part of a much more extensive feature set that allowed additional OSD configuration abilities similar to what can be done with mpv's OSD configuration.

Complete tangent, but such work would be the perfect time to migrate the OSD to a more modern framework like SwiftUI or Javascript...

Do you want to take on adding Enable OSC?

I think that is one of those features where 90% of the effort is modifying the settings UI. I can work on implementing it sometime in the next few weeks if you like.

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.

HIDE FILE NAME IN UPPER LEFT WHEN OPENED IN FULLSCREEN
3 participants