lxqt / pcmanfm-qt

File manager and desktop icon manager (Qt port of PCManFM and libfm)
https://lxqt-project.org
GNU General Public License v2.0
416 stars 113 forks source link

Separate the pcmanfm-qt for lxqt-desktop and qt-pcmanfm #358

Closed fiszts closed 6 years ago

fiszts commented 8 years ago

What about to separate the qt-pcmanfm for a different processes, like xfdesktop as desktop manager in XFCE and thunar as file manager for those who want to use different file/desktop managers, such as openbox/qtfm/thunar/dolphin etc.

palinek commented 8 years ago

I don't get this. pcmanfm-qt is completely independent of all other lxqt components. You may use any other file manager if you wish.

fiszts commented 8 years ago

pcmanfm-qt includes the file manager and desktop manager (icons, d'n'd, gvfs, trash bin etc.), it is combined in one process, if you gonna switch to a different file manager pcmanfm-qt will still provide file manager and desktop manager.

agaida commented 8 years ago

and the problem is exactly what?

fiszts commented 8 years ago

The problem is separating the desktop manager from the file manager. lxqt-desktop for desktop manager and qt-pcmanfm for default file manager, as I wrote for example in XFCE: "xfdesktop" is handling wallpaper changing, arranging icons (computer, home, trash etc.), gvfs (usb sticks mounting, remote filesystems mounting) and there are especial file manager called "thunar" for all the file operations.

tsujan commented 8 years ago

@fiszts That seems reasonable to me. However, it may be as hard to implement as it's easy to say. I'll think about it but will give no priority to it.

agaida commented 8 years ago

To be true - i don't see a real benefit, it's not unusual that the file manager paint the desktop - nautilus does this too. And the process that paint the desktop is separated from the file managing process.

tsujan commented 8 years ago

@fiszts This isn't the first time a file-manager is a desktop manager too. See Nemo!

@agaida We should see if there's any practical advantage to separate them from each other. Nothing comes to mind for now; so, I agree with you. But the idea was interesting.

agaida commented 8 years ago

@tsujan - if it make a real difference or give a real advantage - i'm all for it, but right now i see no benefit.

fiszts commented 8 years ago

Well, as LXQT About says: "A lightweight, modular..." Yes, it's lightweight for sure! That's the reason why I'm added this issue, I'm planning to switch to LXQT, but is it really "modular" if the file manager are "stucked" in desktop manager, what if somebody's just want it to complitely remove or use for example an Openbox context menu and only terminal without file manager at all or to switch to another fm, or some forks of pcmanfm like stuurman or SpaceFM?!

palinek commented 8 years ago

what if somebody's just want it to complitely remove or use for example an Openbox context menu and only terminal without file manager at all?!

He/she surely can...i'm an example of such user :smile: What problem do you have with disabling the pcmanfm-qt as desktop painter?

agaida commented 8 years ago

In debian it is easy - apt purge pcmanfm-qt - so i dont get the point right now. In the beginning of LXQt I'm used to use the LXDE file manager (pcmanfm) to paint the desktop and pcmanfm-qt for file management. So i really don't get it.

fiszts commented 8 years ago

Ok, my last attempt, I googled and found guy "geekless", but it's for LXDE (git: https://github.com/geekless/stuurman-desktop). So that's exactly what I'm talking about, He's just ripped desktop manager from pcmanfm! Anyway it will be nice for me if you're gonna make similar for LXQT, couse this is MODOOLAR! Peace! :smile:

agaida commented 8 years ago

ah, a very nice (and dead) example of bike shedding

tsujan commented 8 years ago

I mostly agree with @agaida and @palinek.

The reason I tagged this with the "later" label was that, even with the current source, the codes for desktop and folder-view are somehow separate from each other because they serve two different purposes. Hence, I consider the idea reasonable. However, being just reasonable is not enough, IMO.

@fiszts pcmanfm-qt is already lightweight. Separating its desktop module from it would have no tangible effect on that. You could use or ignore the desktop functionality easily. And the fact that someone has separated them in the case of LXDE can't be a reason why we should do so too.

I don't know -- maybe, later pcmanfm-qt won't be so lightweight and then we'll have a practical reason to separate its desktop module.

All that being said, let's keep this discussion open for now.

agaida commented 8 years ago

The situation right now: 5128 -rwxr-xr-x 1 root root 451K Mai 21 14:42 /usr/bin/pcmanfm-qt 393358 -rw-r--r-- 1 root root 635K Mai 29 02:29 /usr/lib/x86_64-linux-gnu/libfm-qt.so.3.0.0

[sarcasm]if we rip out the desktop painting thing i guess we have two binaries with 430 k and the lib - so it really make sense :) [/sarcasm]

EDIT: Sorry, missed the sarcasm tags

fiszts commented 8 years ago

Moreover it is easier to customize/edit/recompile 2 different parts. And the fact that this discussion continues means something :smile:

pmattern commented 8 years ago

I'm under the impression you missed the crucial point here. The desktop is not handled by PCManFM-Qt per se but by a dedicated instance which is launched with switch --desktop, see man 1 pcmanfm-qt. This instance can run in parallel with all kinds of other file managers but launching this instance is in no way obligatory. This means you are really free to choose by now already. PCManFM-Qt can be used as file manager without managing the desktop e. g. on any other desktop environment or in a purist window manager only session. On the other hand you are free to use any arbitrary file manager while PCManFM-Qt does handle the session. It is even possible to use an arbitrary tool other than PCManFM-Qt to handle the desktop within an LXQt session. You just have to disable the so-called LXQt module "Desktop" in "LXQt Session Settings" (binary lxqt-config-session) - Basic Settings. So the only potential problem are technical issues which I for one am not aware of. In particular the instance pcmanfm-qt --desktop does not have a large footprint.

tsujan commented 7 years ago

I changed my mind: Desktop should be separated from pcmanfm-qt and rewritten based on libfm-qt. There are, at least, two reasons for that:

(1) Desktop has a design problem (@PCMan is aware of it): It duplicates some sensitive codes of libfm-qt. (2) Users might want to use another file manager under LXQt while having its desktop.

That being said, if pcmanfm-qt and desktop are separated from each other, pcmanfm-qt should have a daemon mode (unlike Dolphin).

A plan for distant future, perhaps...

PCMan commented 7 years ago

OK, time to explain the logic behind this design decision.

  1. You actually get the desktop manager "for free" and stripping this part does not save much. The desktop icons are no more a special version of an ordinary file manager window with its background painted differently and its menu & toolbars removed. Most parts of the desktop manager actually reuses the code of the file manager windows resides in libfm-qt. The high resource usage of the desktop manager is almost always caused by the wallpaper pixmap. Change your background to solid color without a wallpaper to prove that. Or you can use a memory profiler like "massif" to prove that, which I did during the development.

  2. In addition, "cold start" of a file manager with moderate features is slow no matter how you optimize it since it's an I/O bound task. So many file managers offer a daemon mode that runs in the background so every time you open a folder it can show up immediately. That means, your file manager is always running no matter it manages the desktop or not unless you prefer slow cold startup and worse UX. On the other hand this makes FM as desktop manager a perfect option since you need to make it constantly running anyways.

  3. The most memory hungry parts of a file manager are actually the file "icons". Making the code size small won't save memory footprint. Making the file icons small will. Again, please feel free to use a memory profiler if you need some proof. Remember that these QIcon-occupied memory are private to each process. So having two processes loading the same set of icons double the memory usage.

The conclusion are hence simple, less processes, less memory usage. Running the desktop manager is effectively running the file manager in the background as a daemon. The only difference is it paints the background with a memory hungry large pixmap. So the benefit of merging these two components is quite obvious for a lightweight desktop.

PCMan commented 7 years ago

If you don't like the desktop manager, just turn it off from lxqt-config-session. If you don't like the file manager, just uninstall it and use another one, but don't bother if you just want to save dozens kilobytes file sizes. When in doubt, a profiler can help a lot and I highly recommend "valgrind massif".

tsujan commented 7 years ago

If you don't like the file manager, just uninstall it and use another one

If the user doesn't want the file manager, he couldn't have the desktop either. I think all users are aware that they could uninstall anything. This is about violating modularity, not just about personal preferences.

Since DesktopItemDelegate is more or less a duplicate of libfm-qt's FolderItemDelegate, the desktop code is already like a patch inside a whole.

In addition, "cold start" of a file manager with moderate features is slow

A daemon mode will surely be needed but, the first launch aside (which is slow with desktop mode too), launching is very fast in the case of pcmanfm-qt. I've tried it under E.

palinek commented 7 years ago

I fully agree with @PCMan here. The only thing I can see here as a "modularity" improvement is to allow opening of folder on desktop by different FM.

agaida commented 7 years ago

nope - it doesn't violate the modularity thingy - it's about to make clear, what some programs do. Example: The first siduction release back in 2014 contains pcmanfm and pcmanfm-qt - pcmanfm because the desktop painting with pcmanfm-qt at this time was a little bit buggy - works fine :)

@palinek - such things would be at least questionable - mime handling and such things would make it hard (only a vague guess) @tsujan - for me is the desktop a folder, ok, a special one, but pcmanfm-qt is fine for me in that regard.

tsujan commented 7 years ago

OK. This is the situation:

Users want more modularity. @palinek and I see their point (while @palinek doesn't agree with me otherwise). @PCMan and @agaida don't agree that there's a modularity issue.

IMO, this is about modularity and modularity is worth using a few Kb of RAM and hard.

My other argument was (and is) about the code. Desktop is so different from other places that, even now, its code is like a patch.

tsujan commented 7 years ago

Sometimes, while discussing about RAM and CPU, they are treated in such a way as if we're in early 1980's. Of course, we should keep pcmanfm-qt lightweight but lightweight according to the current standards, not for an old PC belonging to 80's.

agaida commented 7 years ago

so let me clarify - i'm not against any changes or against improving modularity. So if it is feasible to have a program $paintdesktop instead of pcmanfm-qt --desktop - why not, if it is possible without much duplicated code and maintenance burden. From the packaging point of view it should be no problem to create two binary packages instead of one out of the source.

tsujan commented 7 years ago

So if it is feasible to have a program $paintdesktop instead of pcmanfm-qt --desktop - why not,

Everything is possible with an enough amount of dedication ;)

if it is possible without much duplicated code and maintenance burden

That's one of my arguments. There is duplicated code already. I can imagine a separate desktop manager that uses libfm-qt without reinventing some of its code. That would unclutter pcmafm-qt's code too.

PCMan commented 7 years ago

@agaida @palinek @tsujan I got a new proposal To solve the modularity issue, we can make the desktop manager a plugin module. So it can be installed separately when needed while things are still kept in the same process. Making things in the same process not only saves memory but also speed up initialization, which is quite slow for Qt. If we want to make them fully usable separately, this can be achieved by making both parts plugin modules, and use the same process to launch them. So we get the modularity without sacrificing the advantages we currently have. About memory usage, in gtk+ the memory used by icons might be shared among processes if a gtk-icon-cache file is generated for the current icon theme. Qt however does not support using this kind of approach. These pixmaps take dozens of MiBs, not several KiBs. In addition to icons, there are still more cached data that cannot be shared across process. With things staying in the same process, you get faster and lighter apps.

tsujan commented 7 years ago

@PCMan In your proposal, can desktop be used without using the file manager? I ask because that's the main idea here; installing an extra package as a dependency isn't important, IMO.

PCMan commented 7 years ago

@tsujan About the sorry state of the desktop manager, I really agree that we should develop a new one. I evaluated several solutions previously but with Qt we don't have many options. I intended to reuse existing code initially and later noted that QListView lacks many features and flexibility we want. That's why you see all the manual icon positioning hacks and others. I hate these hacks, just like you. A potentially good option is taking the old desktop manager code from razor-qt, but it's implemented using QGraphicsView whose use is discouraged by Qt developers. That widget can be considered deprecated. Also it does not have accessibility support so blind users cannot use it. QListView on the other hand has it and is compatible with screen readers. That's why I tried very hard to keep using QListView even hacks are needed. The most reasonable option nowadays should be QML, which also support QAbstractItemModel. So we can try to develop one using libfm-qt. Let QML handles the painting part and Fm::FolderModel can still be used to provide the data. The only drawback of this approach is, we need to load the whole V8 javascript runtime and QML/QtQuick just to paint the icons.

tsujan commented 7 years ago

@PCMan Recently I had to deal with item view code (at https://github.com/lxde/libfm-qt/pull/67) and found that is still has potentials. In fact, I think Qt does have many options.

If you want, I could work on removing the duplicated code -- which isn't related to having a separate desktop manager.

PCMan commented 7 years ago

@tsujan This requires some more work but technically it's possible. Things need to be split into three parts.

  1. plugin_loader (executable file which provides main() function)
  2. pcmanfm-qt plugin (*.so)
  3. desktop plugin (*.so)

Let plugin_loader implements a dbus method "load_module()" Users who want the FM can install 1 + 2. Others install 1 + 3. The command pcmanfm-qt calls the dbus method of plugin loader to load its module. The same for the desktop manager.

tsujan commented 7 years ago

@PCMan Good plan!

PCMan commented 7 years ago

@tsujan Yes if you see anything you can improve, any patches are really welcome and appreciated. But I'm quite busy lately. Also the problematic mess created by my c++ 11 attempt is not resolved yet. That's really unfortunate. It seems that instead of fixing existing problems, I brought more new problems with these changes. So, after some more thinking, not merging all the C++ port mess and just give them up is also one of the options I can accept. After all the whole project and other developers who are willing to work on it should not be blocked by me. @tsujan If you have some improvement and other developers agree with your changes, please feel free to skip me and go ahead to merge them. It's no longer my project. It's "ours". :)

tsujan commented 7 years ago

If you have some improvement and other developers agree with your changes, please feel free to skip me and go ahead to merge them. It's no longer my project. It's "ours".

But what about your project at https://github.com/lxde/libfm-qt/pull/63? Completing it is a must, IMO, and it's a big project. If I merge anything, rebasing of that PR will be difficult.

The most reasonable option nowadays should be QML, which also support QAbstractItemModel.

Oh, no, please! I have many reasons why QML is a bad choice. Long story short, it's doesn't obey the style engine.

IMO, the problem is about duplicated code. The current code manages desktop smoothly. Yes, there are some practical problems but they can be fixed.

PCMan commented 7 years ago

@tsujan BTW about the desktop manager, if you're interested in working on that, I'd suggest start a new project using QML to paint the UI and libfm-qt to provide the folder content. That should be the most time-saving way to give the most beautiful UI/UX with less coding. Not sure about the resource usage and accessibility support, though.

tsujan commented 7 years ago

I'd suggest start a new project using QML...

Please accept my hatred of QML as a personal attitude ;) It's a mess in Qt, IMO. KDE has created many problems and lost many features by using it.

As for me, I'm interested in all of LXQt, especially pcmanfm-qt and libfm-qt -- as my PR's show. I really hope that you'll complete https://github.com/lxde/libfm-qt/pull/63. Yes, I disagreed with it at first, but later I realized that I was wrong. It's expected that a huge change like https://github.com/lxde/libfm-qt/pull/63 introduces serious regressions and if I were you, I'd do it as small PR's. However, its bugs can found and fixed one by one. That project might fix serious issues that may not be able to be completely fixed in libfm.

agaida commented 7 years ago

sounds all good for me - but i can't say much about implementation details because of lack of knowledge

PCMan commented 7 years ago

@tsujan (shake hands) I hate QML a lot as well. However, you know I love numbers and profiling. If actual experiments show that it works well for our task, I'll happy accept it. It's a pity that we cannot use QGraphicsView. It's created for the problem we are solving now and is the best fit technically. But the lack of accessibility and the deprecated state make it no option. :-( FYI: http://blog.qt.io/blog/2017/01/19/should-you-be-using-qgraphicsview/

tsujan commented 7 years ago

@PCMan

I hate QML a lot as well

Hurrah!

If I may encourage you in completing your project: Specifically, it might fix https://github.com/lxde/pcmanfm-qt/issues/397 and https://github.com/lxde/pcmanfm-qt/issues/460, which are serious and about which I'm sure.

tsujan commented 7 years ago

But you might want to redo it as small PR's -- just my 2 cents.

palinek commented 7 years ago

If I may encourage you in completing your project: Specifically, it might fix #397 and #460, which are serious and about which I'm sure.

Isn't a merge in state "as is" an option? I mean, we can release the current master, merge the C++11 branch and regular "bleeding edge" users will report problems which we can solve one-by-one until we're ready for next release. (and maybe create a new "stable" branch for "stable" downstreams where any eventual fixes can be done)

And in the meantime we are (I mean you :) ) free in adding new features w/o the need of any difficult merges between branches.

tsujan commented 7 years ago

Isn't a merge in state "as is" an option? I mean, we can release the current master, merge the C++11 branch and regular "bleeding edge" users will report problems which we can solve one-by-one until we're ready for next release.

Oh, it has very serious problems! Users would get headaches and uninstall pcmanfm-qt instead of reporting bugs ;) I couldn't use it either because I need a stable enough FM with nice features pcmanfm-qt has currently (some of which are in my PR's).

IMO, if it's possible, the same experiment could be done as a series of small enough PR's, each of which appearing after the previous one is tested, debugged and merged. By "small enough" I don't mean really small but manageable to be read, tested and debugged.

EDIT: I don't know about others but, personally, I'm not able to read a PR, predict its possible regressions and debug it when it's so big that it seems like a new program.

agaida commented 7 years ago

@tsujan - i somewhat like the idea of merging it as is - i wouldn't release it official with whistles and bells - maybe as an "experimental" release or an internal release. So people can jump on the band wagon or not. For debian and derivatives i guess it will be 'or not' because we are in a freeze.

Edit: I think thats what i meant with "lets break some things" - users of bleeding edge will be reminded what bleeding means, regular users are not harmed.

tsujan commented 7 years ago

@agaida, have you tried it as your only file manager? I think a developer can be seen as a "terribly bleeding edge user" and I'm one. I tried but I couldn't use it. Without using it, testing doesn't have much sense because an FM has so many aspects.

Since I'm very familiar with the bugs it might fix, I really like it to be completed and merged but I have no clue about how the bugs/regressions, that it'll introduce, could be fixed because it's huge. In that state, only @PCMan could do something about it -- and that would be a huge amount of work for him -- but if it was done as a series of smaller PR's, he wouldn't be alone.

That being said, I'll follow any decision that @PCMan might take about it.

agaida commented 7 years ago

pcmanfm-qt is my only file manager for years ...

tsujan commented 7 years ago

pcmanfm-qt is my only file manager for years

You mean you're using https://github.com/lxde/libfm-qt/pull/63 without any serious problem?

tsujan commented 7 years ago

Oh, a misunderstanding! I meant that PR. pcmanfm-qt is also the FM I use. I never develop/patch an app that I don't use.

agaida commented 7 years ago

but i have a maybe crazy idea - i have a sid based repo ... - and it should be no problem to build with this patch and upload it - it the result is to horrible - ok, back to the older code with a higher version.

tsujan commented 7 years ago

OK. If you could use it on a regular basis too, you could write the best bug reports. I really need to have the current git pcmanfm-qt to work on it and with it.