In T12815#238382, @marcboocha wrote:

Just wondering, is there any use of karbon inside the suite?

Ping, it should be committed?

I test, current situation, i don't think the parch is needed as is. 2 monitors FullHD and 4K (HDMI) now KScreen does not allow different scale factor to distinct monitor but you can change the kdeglobals

[KScreen]
ScaleFactor=1
ScreenScaleFactors=eDP1=1;HDMI1=2;VIRTUAL1=1; # NOTE 2 on HDMI

That seems to work, but not well enough. When you move window from low res monitor to high res one window is scaled automatically, but it seems glitchy. Window content is looking a bit broken (font is scaled and readable well)

New patches go in https://invent.kde.org/ You can fork https://invent.kde.org/utilities/yakuake apply patch and make pull request.

Ping, do not forget.

@ngraham i don't think it's needed this for existing reviews, it's double work for nothing.
It has 2 weeks testing before 5.71 release just push it, it can be reverted if broke something.

In D29833#673879, @ZaWertun wrote:

Another approach is to remove prefix akonadi/ from all calls to the StandardDirs::locateResourceFile (second parameter):

Wait @davidedmundson to accept it.

So change should be fine, (https://phabricator.kde.org/source/plasma-workspace/browse/master/appmenu) since KDED loads only modules they don't know where and when qApp is instantiated.

Is that KWin that set titlebar menus?

But I'm not quite ready for 5.70 yet, that would probably mean recompiling most of KDE

Commented code should be removed.

In D15111#672155, @boemann wrote:

Even for line thickness it doesn't make sense to enable it in general. the image will change if you zoom in and out.

I miss something here, the idea behind the patch is to have pixel metrics in thickness, geometry etc. @boemann i got your opinion as to have configurable pixel units, is that right?

Rebase on master

Thank you, i should wait ship it, but just got Dag comment as it is.

Should it /var/lib/flatpak (or just /flatpak/) be included as well?

Seems fine, i'll push pageapp/flake changes as part of this review in 3.2 branch and master, refactoring main window in separate commit master only.

@danders i saw your patch on my email. d->viewportWidget->canvas()->removeEventFilter(this); fixes the issue but i still prefer all of refactoring code. Please test on all components does not have regressions.

We should call removeEventFilter or found exactly why signal is emitted in destructor.

d->rootPart->createView(doc, this); Creates the view which parent is main window.

Add more context

I don't see why qApp is not deleted here
https://api.kde.org/frameworks-api/frameworks-apidocs/frameworks/kdbusaddons/html/kdbusservice_8cpp_source.html#l00099
Probably because it can be allocated on stack.

KNewstuff is framework, it has only master.

QSignalBlocker ?

Is this still WIP, it looks good, can we proceed?

The idea is to remove some of the configuration possibility: for example only allowing the KoModeBoxDocker to be a left or right sidebar. (putting at the top and bottom was completely broken anyway so I don't think many people did it).

I don't know why everyone wants tools to be above edit window. That's pretty old style when resolution ratio was 4:3 that's why they was arranged one above another. But i agree that it should be flexible to be horizontal or vertical as needed.

What kind of GHNS dialog you spot about? It's a service menu, right click on supported file format (deb, rpm, zst)

+1, well done. Wait ship it from maintainer.

You can rename summary to inform that Debian, RPM and Pacman packages are supported.

In D29028#653194, @romangg wrote:

In D29028#653192, @apol wrote:

I don't really see why we'd want to support something that is not offering ABI stability and doesn't push Plasma in any direction.

It is offering in the same way ABI stability as most other components of Plasma, i.e. until a new minor Plasma release.

+1, looks great.

In D28817#647912, @bport wrote:

I don't think we have a leak, on destructor we delete all view
qDeleteAll(m_views);

Create patch like this git diff -U999 > 1.patch it will add more context to be shown to reviewers.

If you want to kill the job how this loop will be break?

@elvisangelaccio this peace of code is purely wrong at least m_storages is not updated to new device and not only. This code should never exists or try to hide some other bug.

In D28286#641165, @dfaure wrote:

That wouldn't work either, you need to be able to choose between a Notification delegate, a Dialog delegate (which lives in a different library due to the QtWidgets dependency), and some more.

OK, let's keep things as they are. Because the best i can make without dedicated function is new KIO::ApplicationLauncherJob(service, KIO::ApplicationLauncher::WITH_AUTO_ERROR_HANDLED_DELEGATE)

Ok that's look good to me. We can move that code in constructor just before loop for storages, then use m_mtpdevice instead of raw device, but it looks like removed code is just a noise and it shouldn't present at all.

And what about the idea to pass delegate to job constructor? At least it's better than current one. I'm pretty pedantic about duplicate code in plus it makes porting harder.

In D28535#640699, @feverfew wrote:

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

@feverfew are you gonna try what i'm writing about or i should do it? Just use cached device, do not reopen since it'll return nullptr.

I see we don't speak in same language :)
LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
returns nullptr that's normal since device is inaccessible, i mean it does not need to call LIBMTP_Release_Device using m_mtpdevice is safe it's not nullptr, it's just a disconnected device and libmtp knows that.

In D28535#640672, @fvogt wrote:

If getDevice() returns nullptr, this means that MTPDevice::getDevice() returns nullptr. This can only happen if m_mtpdevice is nullptr, which will crash in MTPDevice::~MTPDevice sooner or later anyway.

You're right about bug report, but it can fail in any other place, just in particular version it happen in updateStorageInfo Can we cache getDevice in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device and will not crash.

In D28286#636881, @dfaure wrote:

Why not -> because it just doesn't scale. 30 jobs * 4 delegates = 120 wrapper methods...