| davidedmundson | |
| apol | |
| mart | |
| broulik |
| Plasma |
PBI comes as two parts, the host and the browser extension which come from different sources.
This daemon notifies the user when the has the host installed, but is yet to install the browser side extension a "broken", but likely setup. It then provides a link to the relevant browser store.
In an attempt to not be too annoying this only appears a few times then removes itself.
I'm away next week, Kai can you commandeer - I don't like the module names.
| No Linters Available |
| No Unit Test Coverage |
The code looks fine to me. I was questioning if config.writeEntry("shownCount", 100); should be MAX_SHOW_COUNT + 1 instead for a moment, but this is safer in case it gets bumped later.
I'm not sure if this should be an SNI or a persistent notification (which however I guess we don't really have currently ...) instead. Then again it fits the pattern of things like update modifiers.
I'm left to interpret what David meant with "I don't like the module names".
How about we rename the module to 'browserintegrationchecker'? Let's not bikeshed too much though.
| advert/browserlaunchwatcher.cpp | ||
|---|---|---|
| 62 ↗ | (On Diff #33632) |
namespace { struct BrowserInfo { ... }; const QHash<QString, BrowserInfo> s_browsers { .... }; } |
Nightly users shall suffer! Icon mismatch is not fatal error, at least nightly users should know what they have signed up to :)
I don't think so.
For example, let's say I installed only Google Chrome (from Dev Channel). The only google chrome icon I have is google-chrome-unstable (with different sizes), right? Now, what would I see when I run google chrome? ;-)
I think it would be better to read icons from browser desktop files(e.g. firefox.desktop, nightly.desktop, google-chrome-unstable.desktop, etc). That way, icons aren't hard coded anymore, which is good! Also, m_browsers could be simplified a little bit, e.g.
const QHash<QString, QUrl> s_storeUrls { ... };
FWIW icon was coded like that in order to use a custom firefox + Plasma combined logo / custom chrome + plasma combined logo as appropriate.
Though I'm not sure if that image actually got merged into breeze, at which point we could do anything else.
I'm not a fan of the combined icon idea. On face of it it's a neat "go the extra mile" kind of thing, on the other hand it creates follow-up work which I'm certain will not be done in a timely manner, or even forgotten about: Updating those assets whenever the upstream branding is revised. It means initially the visual is slightly nicer, but in the future it will look outdated and crap. It's not a good idea to bring this about for such a minor thing, I think it's better to keep it simple and agree with Vlad here.
At the same time, I'm not sure using the browser icon straight in an SNI is appropriate either. Upstreams might not appreciate their branding being slapped onto something they didn't originate. I think we should use internet-web-browser here (the same icon that's used for the default browser KCM in System Settings).
Add .desktop file names for Firefox Nightly and Google Chrome Beta.
"Nightly" is the WM_CLASS used by Firefox Nightly. Its .desktop file
should therefore be nightly.desktop, otherwise there are already
integration seams.
The code simplification in the above changes also gets rid of the possible C++20 usage.
Please re-review now.
| reminder/browserintegrationreminder.cpp | ||
|---|---|---|
| 139 | auto usage is not consistent. Also, it would be better not to use auto to deduce raw pointer types, e.g. it would be much better as this auto *menu = new QMenu; auto *action = new QAction(i18n("Do not show again")); | |
There's some conceptual problems with this remaining:
These are somewhat well fixable, I'll see about doing some more work on it this week.
Always open the extension URL in the browser that was actually launched,
instead of relying on it being the same as the default browser.
Don't show the SNI if we can't find the browser to launch it.
- The SNI doesn't disappear when the browser is closed.
This no longer matters in the latest revision, because the SNI will now run the launched browser with the URL as argument. If it's been closed it'll just reopen.
- When you click the SNI to open, it won't necessarily open in the browser it notified about if it's not also the default browser.
Fixed in last revision.
- General issues with using more than one browser simultaneously.
This is still sort of a problem. After installing the extension for one browser, we stop caring about others, so if a user switches browsers or uses multiple browsers regularly they lose out. It could be that's a niche scenario and this is already good enough, though.
I currently don't have the time to make the showCount stuff per-browser to handle multi-browser usage. I think it might also not be worth it. People who use multiple browsers are probably power user enough to hear about and install the extension themselves.
Please re-review now so we can get it in for 5.13.
Thanks a lot!
| reminder/browserintegrationreminder.cpp | ||
|---|---|---|
| 134 ↗ | (On Diff #33877) | There's a plasma-browser-integration icon in breeze-icons now |