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

Fix failure to open smb:// caused by incorrect file info handling. #210

Merged
merged 2 commits into from Jun 10, 2018

Conversation

8 participants
@PCMan
Copy link
Member

PCMan commented Jun 9, 2018

The issue is caused by multiple problems.

  1. in libfm, shortcuts targeting smb:// is also detected as a dir but libfm-qt does not do this
  2. when smb:// is first accessed, gvfs will raise the error: path is not mounted. In libfm it just treat all unmounted paths as directories, which is not correct but can workaround the smb:// case. With the hack, smb:// can be detected as dir correctly even when it's unmounted. Libfm-qt does not contain the hack.

Instead of adding the hack, I implement error handling for the "not mounted" error so smb:// can be mounted automatically upon the first access. This should IMO be the more correct behavior.

@PCMan PCMan requested review from tsujan and agaida Jun 9, 2018

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

@PCMan ComputerFile System is correctly seen as inode/directory but it can't be opened now (the specified directory 'computer:///root.link' is not valid + file monitor cannot be created: Not supported).

It can be opened with 658642a (if that helps).

@PCMan

This comment has been minimized.

Copy link
Member

PCMan commented Jun 9, 2018

@tsujan Ouch! Let me take a look at computer:///.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

@PCMan The mount points under Computer are all OK. It's just that File System can't be opened; it isn't a mount point anymore -- and that's correct.

@PCMan

This comment has been minimized.

Copy link
Member

PCMan commented Jun 9, 2018

@tsujan fixed. I made root.link a mount point. It's also a dir, too.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

@PCMan I think the the last commit (cc37b6b) should cause a regression because it reverses b348ab6 (there's a goto).

@PCMan PCMan force-pushed the pcman@fix_smb branch from cc37b6b to 7031990 Jun 9, 2018

@PCMan

This comment has been minimized.

Copy link
Member

PCMan commented Jun 9, 2018

@tsujan Thanks for the catch. Fixed. It's really bad to use goto :-(
For this part, previously I just copied the old C code from libfm and convert them to C++. Newly written C++ code should not contain goto anymore.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

@PCMan Thanks!

IMHO, goto is a good thing when needed and, especially, I like the way you used it in libfm.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

I confirm the the issue with File System is fixed while it's inode/directory :)

I can't test smb. The crucial test will be @agaida's.

@agaida You'll need to recompile pcmanfm-qt after this.

@HelixBot

This comment has been minimized.

Copy link
Contributor

HelixBot commented Jun 9, 2018

no problem - bah, i should switch my github login soon - when the weblate commits run with the right user

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

@HelixBot

This comment has been minimized.

Copy link
Contributor

HelixBot commented Jun 9, 2018

in the moment - yes: setting up the bot user for the weblate commits :D

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 9, 2018

And I was happy that @HelixBot joined LXQt... I hope you don't have another username, like shapeshifter_at_github.

@yan12125

This comment has been minimized.

Copy link
Member

yan12125 commented Jun 10, 2018

Based on my testing, this also fixes connecting to an SSH/SFTP server via Go -> Connect to remote server in pcmanfm-qt! (with lxqt/pcmanfm-qt#694 applied)

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 10, 2018

@yan12125 That was very good news. Did/could you test smb too?

@yan12125

This comment has been minimized.

Copy link
Member

yan12125 commented Jun 10, 2018

Nope as I rarely use SMB. Just tried it - clicking on a random SMB share and a dialog jumps out asking credentials. I assume that's the desired behavior? I don't own an SMB server so I can't test further.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 10, 2018

@yan12125 That's a good sign.

@tsujan

tsujan approved these changes Jun 10, 2018

Copy link
Member

tsujan left a comment

GTM

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 10, 2018

GTM

@agaida agaida merged commit dc7a575 into master Jun 10, 2018

@agaida agaida deleted the pcman@fix_smb branch Jun 10, 2018

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 10, 2018

@agaida I also had to recompile lxqt-qtplugin after this (no icon, no widget style). I think you also need to do so with Debian.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 10, 2018

... Or maybe after another merged PR; I'm not sure.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 10, 2018

don't forget about lximage-qt :P
https://qa.debian.org/developer.php?login=pkg-lxqt-devel@lists.alioth.debian.org <-- the experimental ones

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 10, 2018

Oh, yes, lximage-qt definitely needs a recompilation too.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

don't forget about lximage-qt :P
https://qa.debian.org/developer.php?login=pkg-lxqt-devel@lists.alioth.debian.org <-- the experimental ones

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

ok, all rebuilds went fine - but please - where i find any changed behaviour?

Hmm - ok, found out that at least "connect to server" works more or less fine - if one will call mount a sshfs and display the local root accept as expected behaviour - at least the filesystem is mounted fine and will be displayed if one click on the mount.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 11, 2018

where i find any changed behaviour?

It should be in smb.

@agaida agaida added this to Done in Pull Requests Jun 11, 2018

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 11, 2018

@agaida Are your IDEFIX folders opened correctly now under Network? I'm also curious to know what you meant above by "more or less fine"?

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

i will make a screencast :P

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 11, 2018

i will make a screencast :P

Which means there's still a problem.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

yes - but i have to double check it in Arch.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

https://youtu.be/8RADzce-cMo - click on the network things just open / - but connect to server is right - the ssh part without login

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

https://www.youtube.com/watch?v=YxyaK81UDj8 - connect to server - ssh - ok, the mount is nearly right, target dir not. Fallback and display / isn't right too.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

now to smb:// - typing in smb://$server works fine, shares are displayed - when one connect to a share - / is shown again, but the mount itself is fine. Additional question - why there isn't smb/cifs available in connect to server? https://www.youtube.com/watch?v=krgvrOYYfKA

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 11, 2018

I knew we should wait until you test it but you merged it yourself ;)

@PCMan ?

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Jun 11, 2018

why not? the now current state is clearly an improvement - and it build and didn't segfault.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Jun 12, 2018

the now current state is clearly an improvement

It is, definitely. But the problem you've encountered is important. I remember that you saw a similar issue with my patch too. Both may be caused by a little bug hiding somewhere.

@johndoe71rus

This comment has been minimized.

Copy link

johndoe71rus commented Sep 27, 2018

hi. do you fix "click on the network things just open /" in master?
i start use calculate linux (gentoo) and this is NEW for me.
I just install 0.13.0-r1
https://packages.gentoo.org/packages/x11-libs/libfm-qt
https://packages.gentoo.org/packages/x11-misc/pcmanfm-qt
and i have this problem.
I'm not good at programming and patching. Therefore, the hope is only for developers of the distribution kit.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Sep 27, 2018

do you fix "click on the network things just open /" in master?

It's fixed in the latest git libfm-qt+pcmanfm-qt.

@dracwyrm

This comment has been minimized.

Copy link

dracwyrm commented Sep 30, 2018

@tsujan is there a specific patch that fixes @johndoe71rus problem? These are the three commits I added to the ebuild in Gentoo: https://github.com/gentoo/gentoo/tree/master/x11-libs/libfm-qt/files I searched the commit history and didn't see anything related to his issue.

Or will there be a new release or patch release soon and I don't need to worry about this?

Thanks

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Sep 30, 2018

@dracwyrm Please always try the latest git versions of libfm-qt and pcmanfm-qt if you want to have the fixes. I assure you that he git versions are practically stable and if they have a problem, it'll be fixed soon after you report it. Gentoo is a good distro for that.

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Sep 30, 2018

BTW, commit titles should be short and so, they may not reflect what they do.

@jubalh

This comment has been minimized.

Copy link
Contributor

jubalh commented Oct 25, 2018

Please always try the latest git versions of libfm-qt and pcmanfm-qt if you want to have the fixes. I assure you that he git versions are practically stable and if they have a problem, it'll be fixed soon after you report it.

But we cannot ask distro to always use latest git in their packages. Usually a distro takes the latest stable version and applies certain patches for things that they need fixed. They don't want all the new functionality but have known bugs fixed.

I think @dracwyrm is asking which commits we think might fix the bug so that he can take those patches in. If a commit contains fixes and new features it is their task to seperate the two from each other and apply only the fixes. But pointing to relevant commits helps a lot.

BTW, commit titles should be short and so, they may not reflect what they do.

That's right. They should just give a short description. The explanation or context should be after the title (if needed)
For example:

Fix buffer overflow in MyFunc()

We had an issue that happens when.... (continue explanation)

I like this one

@tsujan

This comment has been minimized.

Copy link
Member

tsujan commented Oct 25, 2018

But we cannot ask distro...

Suggested to the person; don't have a good verbal memory.

I think @dracwyrm is asking which commits...

It belonged to a period when several interrelated issues were being attacked; I couldn't be sure commit-wise.

That's right. They should just give a short description...

Didn't mean that. We thought this commit fixed it. It was necessary but what finally fixed it was beyond a special case.

Actually, I couldn't work on SMB -- that was a big problem -- but @agaida's tests and clear descriptions did the job.

@agaida

This comment has been minimized.

Copy link
Member

agaida commented Oct 25, 2018

Addition: I mentioned it some times - depending on the knowledge of the backporter one will introduce follow-up bugs with wild porting back upstream changes. So: If one port back something - please check for abi-breaks. We didn't bumped the soname for nothing in the upcoming release.

Rule of thumb: If porting back something to libfm-qt/release check for new provided symbols or at least build lximage, lxqt-qtplugin and pcmanfm-qt against the changed lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment