Nyre221 / Kiview

GNU General Public License v3.0
57 stars 1 forks source link

Observations on Qt6 port (on current Neon Testing) #3

Closed RedBearAK closed 7 months ago

RedBearAK commented 7 months ago

@Nyre221

Some things I noticed after trying the new Qt6/PySide6 port of Kiview.

It's a common thing in modern Linux to have single-letter options be preceded by a single dash "-u" but full word "long" options are typically preceded by a double dash "--shortcut". It might confuse people to just use "-shortcut".

I tried "kiview --shortcut" in the shell and there was a minor additional error message that didn't show up when using "kiview -shortcut". It's been a while since the earlier discussions so I'm not actually clear anymore on what the option does.

The circumstances are also unclear but I keep getting the "Error: Invalid Path" thing sometimes, then other times it works fine, even though I didn't select anything in the Dolphin window between the two invocations of Kiview. This seems to be true regardless of whether I create the shortcut with or without the "-shortcut" option. I'm confused by the inconsistency of when the error occurs and when it doesn't.

Tried to disable the Space bar selection mode in Dolphin by disabling that shortcut, but weirdly I still often get those bars showing up in the files pane when using Shift+Alt+Space as the shortcut. Probably some sort of Plasma 6 bug. I think it's Plasma 6.0.2 after I updated the Neon Testing VM to test Kiview. This may be contributing to the appearance of the invalid path error, but I can't really tell.

Leaving the Kiview window open and re-using the keyboard shortcut after clicking on the Dolphin window will create a second Kiview window (and a third, and so on). Given the power of Kiview as a quick file content viewer, maybe this is intentional and a reasonable way for it to work, sort of like Preview.app on macOS rather than Quick Look. But it's different from the way most "previewers" would typically work, which would be to re-use the same window and just replace the contents with the new path. That would require some kind of lock file strategy or something similar. Not sure it's worth the trouble.

The general navigation and close/exit shortcuts all seem to be working like they did before.

Speed of the window appearing seems quite reasonable. I don't have any videos to preview in the VM, but previewing all the file types in the folder from the downloaded zip works just as well as it always did.

The install with the Install.sh script went OK, though there were some non-fatal warnings in orange and at least one deprecation notice where it said an option was always on now and was being ignored. Other than that everything seems fine.

Porting successful. πŸ‘πŸ½

Nyre221 commented 7 months ago

@RedBearAK

It's a common thing in modern Linux to have single-letter options be preceded by a single dash "-u" but full word "long" options are typically preceded by a double dash "--shortcut". It might confuse people to just use "-shortcut".

Changed.

I tried "kiview --shortcut" in the shell and there was a minor additional error message that didn't show up when using "kiview -shortcut". It's been a while since the earlier discussions so I'm not actually clear anymore on what the option does.

That option prevents the Kiview window from appearing if no active dolphin window is found. This was done to prevent the application from showing its "How it works" menu accidentally.

The circumstances are also unclear but I keep getting the "Error: Invalid Path" thing sometimes, then other times it works fine, even though I didn't select anything in the Dolphin window between the two invocations of Kiview. This seems to be true regardless of whether I create the shortcut with or without the "-shortcut" option. I'm confused by the inconsistency of when the error occurs and when it doesn't.

Now the path that kiview retrieved from dolphin is written in the terminal if that error occurs.

Maybe it's a combination of Plasma6+slow virtual machine. I remember that I had to put sleep 0.1 in the QuickView script for this reason.

For now I have put a delay at the end of a function (it was already there before but it was positioned badly)

The install with the Install.sh script went OK, though there were some non-fatal warnings in orange and at least one deprecation notice where it said an option was always on now and was being ignored. Other than that everything seems fine.

Removed.

Tried to disable the Space bar selection mode in Dolphin by disabling that shortcut, but weirdly I still often get those bars showing up in the files pane when using Shift+Alt+Space as the shortcut.

it happened because you had no file selected and "copy file location" makes the bars appear in this case.

Leaving the Kiview window open and re-using the keyboard shortcut after clicking on the Dolphin window will create a second Kiview window (and a third, and so on). Given the power of Kiview as a quick file content viewer, maybe this is intentional and a reasonable way for it to work, sort of like Preview.app on macOS rather than Quick Look. But it's different from the way most "previewers" would typically work, which would be to re-use the same window and just replace the contents with the new path. That would require some kind of lock file strategy or something similar. Not sure it's worth the trouble.

It seems complicated to do and I don't think it's worth doing at this stage, I don't even have a clean Dolphin integration at the moment.

Thanks for the feedback πŸ‘‹.

RedBearAK commented 7 months ago

That option prevents the Kiview window from appearing if no active dolphin window is found.

Oh, right, the problem of overcoming the fact that it's a global shortcut. Now I remember. And I forgot to test without a Dolphin window open.

For now I have put a delay at the end of a function

I'll see if it seems to make a difference.

it happened because you had no file selected and "copy file location" makes the bars appear in this case.

Ooooohh... KDE makes some things much more complicated than they should be.

It seems complicated to do

There's usually a library or something available to handle most of it. Like lockfile in Python. Not part of the standard library though.

Will try to do a bit more testing.

RedBearAK commented 7 months ago

Shortcut option is definitely changed, and the effect is as it should be, if there is no Dolphin window around. πŸ‘πŸ½

It really has to be a (KDE) bug that you can only change the visible name of a command shortcut, and not the actual command it runs, without deleting and re-creating it. Nothing to do with you.

I don't suppose you'd want to also bind Ctrl+W to close the KiView window. I can take care of it myself in my Mac-style keymapper config by just matching on com.nyre.kiview and remapping it to Escape, but I think it's also a fairly common key binding for "close window". Trying Cmd+W (which with the keymapper is Ctrl+W) to close things that my brain thinks of as just a "window" and not a full "application" (where I would use Cmd+Q) is still an unconscious habit from using macOS for so long.

Still getting the invalid path error when nothing is selected, pretty much randomly. I thought maybe that it was more frequent when I didn't hold down the Space bar for a brief second when invoking the shortcut, but after doing it quite a few times that doesn't really seem to have any impact either way. Random.

On the other hand, it doesn't seem to be getting stuck in the selection mode in the Dolphin window as much as it was before. Seems like it was getting stuck with the bars visible about 30% of the time with the version before this. I can't seem to get it to do anything but flash the bars now, and then they go away. Maybe the delay does help in that respect. πŸ€·πŸ½β€β™‚οΈ

Still a few things showing up during the build:

Right after "Found WrapVulkanHeaders" there's some of the orange warnings, and another right after "Found KF6I18n". Sorry I can't copy and paste from this VM (Wayland). Both say at the end: "Use -Wno-dev to suppress it."

Then further along in the build there are two warnings about "unused parameter" in dolphinbridge.cpp. Line 207 if I'm reading the output correctly.

Nothing really seems to interfere with the process completing, or the use of the program.

End of report. πŸ‘πŸ½

Nyre221 commented 7 months ago

@RedBearAK

It really has to be a (KDE) bug that you can only change the visible name of a command shortcut, and not the actual command it runs, without deleting and re-creating it. Nothing to do with you.

I confirm, I had that problem too.

Still getting the invalid path error when nothing is selected, pretty much randomly. I thought maybe that it was more frequent when I didn't hold down the Space bar for a brief second when invoking the shortcut, but after doing it quite a few times that doesn't really seem to have any impact either way. Random.

The path that kiview takes from dolphin should now be written in the terminal, can you check which one it is? (most likely it is empty).

Unfortunately, since this problem doesn't happen to me, I don't know how to reproduce it and fix it.

If you want, you could try increasing the delay in https://github.com/Nyre221/Kiview/blob/master/src/dolphinbridge.cpp (line 192: std::this_thread::sleep_for(std::chrono::milliseconds(100) );) and see if kiview still gives the invalid path problem.

Later I'll try to give less power to my virtual machine and see if it gives me the same problem.

Right after "Found WrapVulkanHeaders" there's some of the orange warnings, and another right after "Found KF6I18n". Sorry I can't copy and paste from this VM (Wayland). Both say at the end: "Use -Wno-dev to suppress it."

These warnings seem to come from external libraries.

Then further along in the build there are two warnings about "unused parameter" in dolphinbridge.cpp. Line 207 if I'm reading the output correctly.

For now I have decided to leave that parameter even if it is unused.

I don't suppose you'd want to also bind Ctrl+W to close the KiView window. I can take care of it myself in my Mac-style keymapper config by just matching on com.nyre.kiview and remapping it to Escape, but I think it's also a fairly common key binding for "close window". Trying Cmd+W (which with the keymapper is Ctrl+W) to close things that my brain thinks of as just a "window" and not a full "application" (where I would use Cmd+Q) is still an unconscious habit from using macOS for so long.

In the future I might put a menu to configure the shortcuts.

Thanks for the help πŸ‘

Nyre221 commented 7 months ago

@RedBearAK

I checked on the virtual machine and the problem was present with the old version of Kiview. After updating to the same version you have (with the delay repositioned) the problem disappeared, but I guess this depends on the speed of the machine.

I'll try weakening the virtual machine again and see if it reappears. At some point I will have to choose a compromise anyway. I can't increase the delay too much and I can't not use it.

Edit:

Even with the super slow virtual machine the delay I used seems to be sufficient... I'm waiting for you to tell me what amount of delay doesn't make that message appear:

If you want, you could try increasing the delay in https://github.com/Nyre221/Kiview/blob/master/src/dolphinbridge.cpp (line 192: std::this_thread::sleep_for(std::chrono::milliseconds(100) );) and see if kiview still gives the invalid path problem.

If you want of course.

Have a nice day

RedBearAK commented 7 months ago

To get the invalid path error to reasonably go away it turns out the delay must be ~500 ms in my case.

By making it 5000 ms (5 seconds) I observed that this delay only seems to happen when it is necessary to do the "select all/deselect all" trick that you're using to try to deal with the error from having nothing selected. Subjectively, I would not be mad about a delay of 500 ms if it meant that I'd pretty much never see the invalid path error. It's still just half a second. In human terms that's not long. Barely an eye blink or two. And when the path isn't invalid, this delay is already irrelevant.

I bet you're wanting to be slick and keep the delay short enough that the user is not aware of the selection trick that is happening, but I don't think trying to hide that really serves much purpose, if it leaves the delay too short to be reliable in the wild. In fact I feel like it's kind of neat watching the selection process in action during that half second. Maybe document it in the README, and those who are curious why it needs to happen can help put pressure on the KDE devs to eventually have a better way to obtain the path from Dolphin when nothing is selected. There really does need to be a better way.

FYI: When trying to run KiView from the terminal after a sleep command, I always see some error about a "cyclic dependency" referencing some QML files, and sometimes lines about "Error executing SQL" and 'Parameter "link" is not declared'. Nothing that seems to interfere with the functioning of the app, but I also never saw any path being printed out in the terminal.

Thanks for letting me test out your updates.

Nyre221 commented 7 months ago

@RedBearAK

By making it 5000 ms (5 seconds) I observed that this delay only seems to happen when it is necessary to do the "select all/deselect all"

Hmm, if it appears even with a 5 second delay then I think the problem is something else. In theory, the delay is to give dolphin time to finish the animation and select all the files, and 5 seconds is definitely longer than necessary.

On my virtual machine (super slowed down) it no longer appeared even with the 100 millisecond delay.

Hmmm, now that I think about it, for Plasma 6 to re-enable "copy file location" when there are multiple files selected I had to use a Dbus call ( resetEnabled ) in the 'sendCopyFileLocationSignal' function, and maybe it doesn't have enough time to re-enable the option and therefore does not copy anything.

Now I'll try putting a delay after resetEnabled and see if that eliminates the problem. If this doesn't work, I want to try putting 0.1 sec delays spread out like in QuickView.

Just to clarify, are you using a virtual machine?

Does QuickView give you the same problem?

When trying to run KiView from the terminal after a sleep command, I always see some error about a "cyclic dependency" referencing some QML files, and sometimes lines about "Error executing SQL"

External errors.

'Parameter "link" is not declared'.

It doesn't appear to me.

Nothing that seems to interfere with the functioning of the app, but I also never saw any path being printed out in the terminal.

The path should appear in the terminal only if it gives you the "invalid path" error.

In this case the path is empty (invalid):

nyre@nyre-ms7693:~$ kiview 
invalid path
"path:  "

Thanks for testing

Nyre221 commented 7 months ago

@RedBearAK

I have updated the source code: line 180: std::this_thread::sleep_for(std::chrono::milliseconds(150)); line 193: std::this_thread::sleep_for(std::chrono::milliseconds(100));

The delay can be increased later, it makes no sense to set it high for now.

edit: I installed (via deb package) kiview on a virtual machine with kde neon user edition (live) and it didn't give me that problem.

If you want to do more testing, you can use the deb package directly: https://github.com/Nyre221/Kiview/releases/tag/v0.8.1

RedBearAK commented 7 months ago

Hmm, if it appears even with a 5 second delay then I think the problem is something else.

No, you misunderstood that part. It was gone after 500 ms, I just set it to 5,000 (temporarily) to get a better understanding of exactly when that delay was taking effect, to see whether it would be reasonable to just leave it at 500 ms for everyone. At 400 ms it was mostly gone, at 500 ms I couldn't get the error to occur.

Just to clarify, are you using a virtual machine?

Did I not say? I must have erased that version of the comment and started over. QEMU/KVM on a Fedora 39 host with Ryzen 3700u (4c/8t). I typically use the defaults created by GNOME Boxes, which is all available cores, and give the VMs 4GB out of my 32GB pretty consistently.

I don't generally find the VMs to be particularly slow unless they are having some technical problem with the video settings in the VM. Video driver is usually Virtio with OpenGL and 3D acceleration enabled. But there is no dedicated GPU on this model. I don't do any gaming or anything like that.

But this particular machine has always had strange random problems with I/O (in multiple OSes and across many different Linux kernel versions) that most of the users and the main dev of the keymapper I use have never been able to replicate. Like there is some kind of hardware or firmware poltergeist that periodically appears and then vanishes at other times. There's never been any rhyme or reason for it, it's not just about CPU usage.

Hmmm, now that I think about it, for Plasma 6 to re-enable "copy file location" when there are multiple files selected I had to use a Dbus call ( resetEnabled ) in the 'sendCopyFileLocationSignal' function, and maybe it doesn't have enough time to re-enable the option and therefore does not copy anything.

Rather than just a delay, what about a smarter attempt to get the path again only if it comes back empty?

nyre@nyre-ms7693:~$ kiview 
invalid path
"path:  "

I can get that to show up if I set the delay back to 50 ms and use:

sleep 3; kiview

But without the sleep delay to let you refocus the Dolphin window before KiView runs, I don't get how you are not just getting the "no active Dolphin window" thing each time instead of the invalid path error.

With the new version of the code and the values you picked I get the invalid path error quite frequently, around 30% of the runs.

Setting either delay independently to something around 500 ms makes it go away. I can't tell without a lot more messing around which juncture is more important. But if I was trying to prevent this issue I would probably use a limited loop rather than a static delay. That way fast machines can be as fast as they can handle, slower machines can take anything up to some reasonable maximum time to keep trying the path copy on an exponentially increasing delay. I might even save that maximum delay to a settings file, so that delay would just always happen first on stubborn machines.

I've had to do something like this (limited loop) in a couple of places to get things like a tray icon to update reliably when a state changes. In my own code. Without it there were just inexplicable failures to get the icon or some menu items to respond in some distros.

Nyre221 commented 7 months ago

@RedBearAK

But without the sleep delay to let you refocus the Dolphin window before KiView runs, I don't get how you are not just getting the "no active Dolphin window" thing each time instead of the invalid path error.

Integrated terminal (F4)

No, you misunderstood that part. It was gone after 500 ms, I just set it to 5,000 (temporarily) to get a better understanding of exactly when that delay was taking effect, to see whether it would be reasonable to just leave it at 500 ms for everyone. At 400 ms it was mostly gone, at 500 ms I couldn't get the error to occur.

Ah that makes more sense πŸ˜‚

But if I was trying to prevent this issue I would probably use a limited loop rather than a static delay. That way fast machines can be as fast as they can handle, slower machines can take anything up to some reasonable maximum time to keep trying the path copy on an exponentially increasing delay.

Very good idea!

Unrelated: I re-added the configuration for flatpak and it's much faster now. I had started using OBS, but now I think flatpak will be the main way to install this application.

Nyre221 commented 7 months ago

@RedBearAK I made the change you suggested.

Edit: https://github.com/Nyre221/Kiview/releases/tag/v0.8.5

RedBearAK commented 7 months ago

Integrated terminal (F4)

Oh, right, that's never been something I found particularly useful so it didn't occur to me that Dolphin actually has one built in. I like my terminal windows BIG.

I re-added the configuration for flatpak and it's much faster now.

I'll have to check that out again sometime. Right now I'm trying to get through doing a hard fork (renaming) of the keymapper I've been working with for the Mac-style shortcuts project. What a pain. Have to learn about a bunch of new stuff to make sure I can actually maintain it as a true fork. πŸ˜’

I made the change you suggested.

Wow. The result is quite weird, in a good way. I can see that it sometimes flashes twice, hard to tell if it ever gets to three times. But it's all happening so fast that all I really see is some flashing of the selection mode bars and the highlighting of everything in the files pane, and then it's gone and the KiView window pops up. Way faster than I was expecting to be possible. Pretty sure it's often succeeding with just one flash, probably almost always with just two.

I tried for a while but couldn't get the invalid path error to appear (which it should never do anyway if you're using a loop with a few tries available, I see 5 in the code which should take care of it), but I didn't even see any kind of long delay at any point, anywhere perceptually close to the 500 ms I was using as the static delay value. Yet it works.

Very interesting. I think that you can probably call that one solved about as well as you can solve it under the circumstances. πŸ‘πŸ½

Nyre221 commented 7 months ago

@RedBearAK

Wow. The result is quite weird, in a good way. I can see that it sometimes flashes twice, hard to tell if it ever gets to three times. But it's all happening so fast that all I really see is some flashing of the selection mode bars and the highlighting of everything in the files pane, and then it's gone and the KiView window pops up.

Party mode πŸ˜‚

but I didn't even see any kind of long delay at any point, anywhere perceptually close to the 500 ms I was using as the static delay value. Yet it works.

With each attempt the waiting time triples: waitingTime = waitingTime * 3;

I started from a very low number (25 milliseconds) to avoid putting an initial delay that was too long for no reason.

I'll have to check that out again sometime.

I don't think there is any need, the package will go on flathub and they will take care of checking for errors or similar. The only logic that can break in flatpak is that of the libreoffice process, but from the tests I've done everything is fine(That part of code is as it was on Qt5).

Right now I'm trying to get through doing a hard fork (renaming) of the keymapper I've been working with for the Mac-style shortcuts project.

Might I ask you why you're making a fork?

Thank you so much for the help you gave me.

Edit: If you want to try the flatpak version, here are the instructions: https://github.com/Nyre221/Kiview/blob/master/extraStuff/BuildFlatpak.txt

As I said before, I don't think there is a need.

Thanks again!

RedBearAK commented 7 months ago

waiting time triples - started from a very low number (25 milliseconds)

Yeah, that was actually a very wise choice for this use case. Start small and then get much bigger fast within the limited tries. Worked out great.

Might I ask you why you're making a fork?

Made too many changes to the keymapper when I was adding some Wayland support and some other stuff. It's highly unlikely the changes can get merged back into the original project, and someone is trying to get my main project working on NixOS. It's more difficult to tell them to clone a specific development branch of my soft fork than it would be to just publish those changes in a new repo under a different name. Just have to figure out what I missed, since it's not working.

Nyre221 commented 7 months ago

Made too many changes to the keymapper when I was adding some Wayland support and some other stuff. It's highly unlikely the changes can get merged back into the original project

Oh, got it.

Just have to figure out what I missed, since it's not working.

Tell me if there's anything I can do to help you. Good luck.

RedBearAK commented 7 months ago

Tell me if there's anything I can do to help you.

Thanks, but I managed to find all my typos and imports that needed to be updated. Working quite nicely now. πŸ‘πŸ½

Still a ton of references to the original project (keyszer) in the README, but that can be cleaned up later.

https://github.com/RedBearAK/xwaykeyz

I don't think there's much else to do to tweak the ported KiView. You can probably close this as resolved if you want.