owncloud / client

🖥️ Desktop Syncing Client for ownCloud
GNU General Public License v2.0
1.4k stars 663 forks source link

Windows SocketAPI named pipe path has fixed "ownCloud" string #6983

Open ckamm opened 5 years ago

ckamm commented 5 years ago

See the SocketAPI constructor. This makes the shell integrations not work properly if branded or derived (nextcloud) clients are running at the same time.

guruz commented 5 years ago

Windows only indeed.. On macOS we use the app domain name: https://github.com/owncloud/client/blob/master/src/gui/socketapi.cpp#L188

I remember there was a reason for this.. maybe because the Windows plugin was built in the 'binary' submodule before, but now that we switched to MSVC we could always build it with the branding instead?

CC @ogoffart @jturcotte @dschmidt

dschmidt commented 5 years ago

FTR probably APPLICATION_SHORTNAME or Theme::appName() should be used instead. The shell extension needs to be adjusted as well then probably.

The reason why this was not branded in the first place was probably that we used to ship binaries of the shell extension instead of building them when building the rest of the client. That made branding it impossible and so always the hardcoded ownCloud name needed to be used. This is obviously not the case anymore.

guruz commented 5 years ago

@dschmidt commented at the same time :D

guruz commented 5 years ago

Hm, the commit above only makes sense if we also change the DLL names, e.g. OCOverlays.dll -> ownCloudOverlays.dll This would involve more changes, for example in the Makefiles and in MSI. Postponing to 2.5.4? CC @ogoffart @dschmidt

dschmidt commented 5 years ago

@guruz What does this have to do with dll names?

guruz commented 5 years ago

Our goal is to have completely different brandings, e.g. if CERNBoxOverlays.dll and ownCloudOverlays.dll diverge. (Also looks less mysterious as a DLL name)

dschmidt commented 5 years ago

Well, it's perfectly fine to do that - but there's absolutely no reason that requires it afaict. It's not like the file is installed to some system directory or anything.

guruz commented 5 years ago

I'm not so sure. I thought we use a single global instance of the DLL (via some identifier). I think @ogoffart @jturcotte know this stuff, I don't.

guruz commented 5 years ago

OK maybe @dschmidt is right, I created a PR for 2.6

HanaGemela commented 5 years ago

I have the below clients running at the same time: Testpilot 2.6.0daily20190429 (build 11645) ownCloud (not branded) 2.6.0daily20190429 (build 11644)

ownCloud client doesn't have the ownCloud submenu

Untitled

HanaGemela commented 5 years ago

Also manual proxy settings are not working when running two clients at the same time

ckamm commented 5 years ago

@HanaGemela Could you make a new issue about the proxy settings? That's unrelated. I'll unassign you since someone else needs to look at this again.

HanaGemela commented 5 years ago

I tried with a daily build from 30 May (VFS off) Icons and submenu in context menu are missing

HanaGemela commented 5 years ago

I've tried using proxy on two clients running at the same machine and it works

guruz commented 5 years ago

https://download.owncloud.com/desktop/daily/ownCloud-2.6.0.12048.11850-daily20190530.msi Windows 10

The icons and the tray menu work for me (I agreed to let the msi reboot)

guruz commented 5 years ago

I removed owncloud and testpilotcloud

Made sure Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\ShellIconOverlayIdentifiers doesnt have owncloud subfolders

Reinstalled https://download.owncloud.com/desktop/daily/ownCloud-2.6.0.12048.11850-daily20190530.msi (incl reboot)

Start owncloud(!!), open sync folder: Icons and Context menu are there.

FYI there is also https://github.com/owncloud/client/wiki/Debugging-Overlay-Icons Maybe someone in NBG can help @HanaGemela ? otherwise we could screenshare.

HanaGemela commented 5 years ago

@guruz Two clients are not working on one machine. Icons are missing. Tried with todays daily builds