It can be disabled, it is done as "plugin" in a similar way as flash player.
It can be disabled by unchecking simple checkbox at:
(Preferences > Browsing > [ ] Allow Pepper Pluginf (Flash player)).

Since this works fine for me, it just needs a better name or description, or secondary checkbox.

In D26753#596575, @SGOrava wrote:

It can be disabled by unchecking simple checkbox at:
(Preferences > Browsing > [ ] Allow Pepper Pluginf (Flash player)).

I think, that checkbox disables/enables another thing: all plugins that use pepper API. It controls the QWebEngineSettings::PluginsEnabled setting, which, according to [1]:

Enables support for Pepper plugins, such as the Flash player. Disabled by default.

This patch, on the other hand, tries to act on another setting: QWebEngineSettings::PdfViewerEnabled, which:

Specifies that PDF documents will be opened in the internal PDF viewer instead of being downloaded. Enabled by default. (Added in Qt 5.13)

Unchecking the checkbox you've mentioned may work because the internal PDF viewer uses pepper API. It is not fair for the user, though, to force them to disable all plugins that use pepper just because of internal PDF viewer.

Making that setting be controlled through another checkbox might be a good idea! The problem is that the setting exists only for Qt 5.13 and above, so I don't know how to make it conditionally used in lib/preferences/preferences.ui...

Links:

1. QWebEngineSettings documentation

It should be optional, many people would like to have internal PDF viewer.

I also agree that current method to enable or disable it is "all or nothing"
which is not great but works for me.

To add checkbox into preferences which will show only in some Qt version is also easy.
You need to create the UI file with all options, than in cpp file check on which Qt version
you are and enable / disable (I don`t know which is better, well there are example like
dns prefetch setting...).

In the end you want download button in internal PDF viewer, it is managed by QtWebEngine
which mean you should also go and bug Qt developers https://bugreports.qt.io/

In D26753#596709, @SGOrava wrote:

It should be optional, many people would like to have internal PDF viewer.

Yes. I've updated the patch. Now it adds a checkbox to choose whether to use internal PDF viewer.

The option is off by default as to keep the behavior consistent with that on older (pre Qt 5.13) systems, but can be turned on.

In the end you want download button in internal PDF viewer, it is managed by QtWebEngine
which mean you should also go and bug Qt developers https://bugreports.qt.io/

Not even them! Qt guys made a wrapper around PDFium (see this announcement), and PDFium is a Google territory. So trying to push a feature through there (upstream) would not necessarily mean that it makes its way into Qt at all.

And my point is not absence of a "download" button in that viewer but keeping the existing behavior regarding PDF documents on newer systems as well as on older ones. A feature I've loved since Qupzilla times. I would not sacrifice an ability to run pepper plugins for that. Instead, being able to opt in/out seems a better solution.

So it is even more complicated with this PDF viewer than I thought.
Thank you for making the option.

So I tested it and it works as intended.
What I see as a problem is that this patch does not honour the relationship between PDF viewer and Pepper plugins.
So if you disable pepper plugins PDF viewer would also be gone and users would complain.

I would add this option under pepper plugin option if possible add some indent and used some signals&slots to make it active only when pepper plugins are enabled and disabled when pepper plugins are disabled.

In D26753#597754, @SGOrava wrote:

So I tested it and it works as intended.

Thanks for testing!

What I see as a problem is that this patch does not honour the relationship between PDF viewer and Pepper plugins.
So if you disable pepper plugins PDF viewer would also be gone and users would complain.

Indeed. I'll change the patch to account for that.

I would add this option under pepper plugin option if possible add some indent and used some signals&slots to make it active only when pepper plugins are enabled and disabled when pepper plugins are disabled.

Notwithstanding the dependency noted above, do you think it is a good idea to put that option directly under "Allow Pepper Plugins (Flash plugin)"? The viewer is not a plugin, in the sense that other Falkon plugins are, or even in relation to qtwebengine, as it is its built-in component within QtEngineCore. What do you think?

I've also noticed the viewer's dependency on v8 in the source code, so even though it appears to work when you uncheck "Allow Javascript" checkbox in the preferences, it may not work entirely as expected. There may be other dependencies we don't know about. All that appears to be internal implementation details prone to change along with "chromium" bundle that Qt takes as a 3rd party component. There seems to be no point in tying all those details on Falkon's top level UI...

I know that.
I still believe we need to do something to make it more obvious or we would just push this work for later date.

I am also not very sure on how to this, I just said my opinion last time but you reject it.

But I still see a need to at least inform user that the pepper plugins needs to be enabled for it to work.
It can be:

• the direct relationship as I said before, we can also think of renaming the pepper plugins option
• Some note under the option
• Or at least tooltip

What do you think?

In D26753#598232, @SGOrava wrote:

It can be:

• the direct relationship as I said before, we can also think of renaming the pepper plugins option

Relationship is now relected by the new checkbox's "enabled/disabled" state, so - done. I think, "Allow Pepper Plugins" reflects it quite adequately, why the need to rename?

• Some note under the option
• Or at least tooltip

How about "(requires Pepper)" in the checkbox caption or a "This requires the option \"Allow Pepper Plugins (Flash)\" to be enabled" tooltip?

In D26753#598233, @alukichev wrote:

How about "(requires Pepper)" in the checkbox caption or a "This requires the option \"Allow Pepper Plugins (Flash)\" to be enabled" tooltip?

It should be very direct and on the eyes.
So I vote for the first option e.g.
"All internal pdf viewer (requires enabled Pepper Plugins)"

In D26753#598233, @alukichev wrote:

In D26753#598232, @SGOrava wrote:

It can be:

• the direct relationship as I said before, we can also think of renaming the pepper plugins option

I think, "Allow Pepper Plugins" reflects it quite adequately, why the need to rename?

On the second thought, I've renamed the checkbox. That makes the new checkbox's dependency part shorter and more clear. I've also moved it directly under "Pepper plugin" one.

OK, works almost fine, only the checkbox disabled state is not triggered at the first launch of preferences dialog.

In D26753#598264, @SGOrava wrote:

OK, works almost fine, only the checkbox disabled state is not triggered at the first launch of preferences dialog.

Oops, seems to be a long time since I've been messing with Qt's event processing. Fixed. Thanks for your testing!

Do you have commit / push rights ?

SGOrava added a comment. View Revision
Do you have commit / push rights ?

No, I don't.

(Apologizing for an apparently unsuccessful reply-by-email attempt.)

I would need your name and email which I can use to commit it for you.
(I can guess, but I want to be sure)
Thank you.

In D26753#598297, @SGOrava wrote:

I would need your name and email which I can use to commit it for you.
(I can guess, but I want to be sure)

Alexander Lukichev, alexander dooot lukichev att gmail dotty com