Skip to content
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

Add embedded terminal support #678

Closed
micrococo opened this issue May 23, 2018 · 23 comments
Closed

Add embedded terminal support #678

micrococo opened this issue May 23, 2018 · 23 comments

Comments

@micrococo
Copy link
Contributor

Hi.

Would it be possible to have a Dolphin/Nautilus like embedded terminal (qtermwidget?) in PCManFM? It is a feature a find quite useful and use a lot. If no member of the LXQt Team is interested in adding this feature, would it be accepted if someone implements it or it is something out of place from the point of view of the desktop vision? My idea is that it should be an option at built time so people not liking this feature could use PCManFM without it.

Thanks in advance.

@tsujan
Copy link
Member

tsujan commented May 23, 2018

Patches are welcome.

@micrococo
Copy link
Contributor Author

That's fine, thanks.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

Isn't that quite useless?
Why not just press F4 to launch your terminal?

@tsujan
Copy link
Member

tsujan commented May 24, 2018

I also think the current behavior is much better because pcmanfm-qt can call any terminal emulator. Moreover, unnecessary complications can't be good.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

Maintaining more code by adding an embedded terminal while there is just the possibillity to launch a terminal sounds wrong to me. If there is a program that does the job it should be used not duplicated.
Actually I'm against such an implementation. Unless there is a good reason.

@tsujan
Copy link
Member

tsujan commented May 24, 2018

My reason was to encourage a developer to get familiar with libfm-qt, pcmanfm-qt and qterminal codes when he's motivated.

@micrococo
Copy link
Contributor Author

@jubalh "Useless" if a subjective term. If it were something useless there wouldn't be any file manager with this feature. It is something that depends on your habits when using a computer. So I think that we shouldn't waste our time trying to convince each other of why this may be a useful feature or not. I've been there before and leads to nowhere...

@jubalh
Copy link
Member

jubalh commented May 24, 2018

That's not what I have been saying at all. Please read again.

@micrococo
Copy link
Contributor Author

I also think the current behavior is much better because pcmanfm-qt can call any terminal emulator. Moreover, unnecessary complications can't be good.

@tsujan That is the reason why I asked if it would be accepted is someone implements it. From the "Patches are welcome", I understand that if I or someone else manages to implement it, it will be accepted. I have little knowledge of the C++ language but I may try to implement my own request if I'm able to do so. Being a build time feature it shouldn't be a problem for the users that don't want it. I'm not sure about this, but I think that even if PCManFM were built with the embedded terminal support, it shouldn't use any extra resources until it is really used (is it called lazy initialization?). And when the terminal is closed the terminal instance could be cleaned.

If it is not going to be accepted I won't even try to do so, but it is fine anyway. It is just a feature request after all.

@micrococo
Copy link
Contributor Author

Isn't that quite useless?

@jubalh I'm not fluent in English, but I think I answered to that. Sorry if I missunderstand what you meant.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

it shouldn't use any extra resources until it is really used (is it called lazy initialization?). And when the terminal is closed the terminal instance could be cleaned.

It is not about hardware resources. It is about developer resources to maintain it in the future. It is about more code creating more chances for bugs. It is about providing what is already there in a slightly different way. About avoiding duplication and having one tool for the job. KISS and Unix.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

Isn't that quite useless?

@jubalh I'm not fluent in English, but I think I answered to that. Sorry if I missunderstand what you meant.

In the comment after that I describe my issue with the request. In the comment quoted here by you I actually said:

Isn't that quite useless?
Why not just press F4 to launch your terminal?

Meaning: Maybe you don't know about the F4 feature, I think it does what you want to. With the slight difference that a new terminal is started instead of using an embedded one. Does this solve your problem maybe already?

That's what I wanted to say with that. Useless in the sense the the function of having a terminal openend in the current directory is available already. Asking what is the difference for you in having it embedded if you think that would improve something.

And all you did was complain about my usage of the term useless without actually answering any important question.

Why not just press F4 to launch your terminal?

So what is the difference of having an embedded one for you?
Is it worth the risk of having more code for the same functionality (for us)?

@micrococo
Copy link
Contributor Author

It is not about hardware resources. It is about developer resources to maintain it in the future. It is about more code creating more chances for bugs. It is about providing what is already there in a slightly different way. About avoiding duplication and having one tool for the job. KISS and Unix.

@jubalh That's the reason why I asked first, to know what the LXQt Team thinks about a feature like this.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

@jubalh That's the reason why I asked first, to know what the LXQt Team thinks about a feature like this.

And when I stated you what I think, and hinted you that there might be a solution for the problem you start saying that I'm degrading your idea. Which I'm not doing. I tried to tell you what already exists and why you think your change would be better.

Please just answer that question.

@micrococo
Copy link
Contributor Author

@jubalh I know about the F4 feature. And I'm not really interested in discussing about the request. I only want to know I patches would be really welcome or not. That's all.

@jubalh
Copy link
Member

jubalh commented May 24, 2018

And I'm not really interested in discussing about the request. I only want to know I patches would be really welcome or not.

That's what I'm trying to find out. Whether such a change would be beneficial or not. To find out whether patches are welcome, one first needs to discuss the purpose of the request...

@tsujan
Copy link
Member

tsujan commented May 24, 2018

From the "Patches are welcome", I understand that if I or someone else manages to implement it, it will be accepted.

@micrococo By "welcome", I meant It will be reviewed. More than that, I also welcomed you to work on pcmanfm-qt.

When I make a PR, I know that it might be rejected by other LXQt members ;) And there have been such cases. It's up to the developer to decide whether he/she wants to spend time on something that might not be merged.

There are always differences in opinions and we know who likes/dislikes what here. So, more discussion on that won't be helpful, IMHO.

@micrococo
Copy link
Contributor Author

And when I stated you what I think, and hinted you that there might be a solution for the problem you start saying that I'm degrading your idea. Which I'm not doing. I tried to tell you what already exists and why you think your change would be better.

Please just answer that question.

@jubalh I didn't mean to say that you were degrading my idea at all. As I say before, I'm not fluent in English and that proves it.

@micrococo
Copy link
Contributor Author

There are always differences in opinions and we know who likes/dislikes what here. So, more discussion on that won't be helpful, IMHO.

I fully agree with that.

@tsujan
Copy link
Member

tsujan commented May 26, 2018

Apart from likes and dislikes, I think @jubalh is right when he says, "It is about developer resources to maintain it in the future." A new feature will be acceptable if it does something that can't be done already, doesn't make the code heavier than needed and is maintainable.

But, most importantly, qtermwidget is hardly maintained until now and so, it shouldn't be used in a clean program like libfm-qt/pcmanfm-qt. Someone should start to understand what happens in deeper layers of qtermwidget and fix its issues.

@tsujan tsujan closed this as completed May 26, 2018
@tsujan
Copy link
Member

tsujan commented May 26, 2018

FYI

Sometimes -- actually, most of the time, I reply to comments while coding because otherwise, I won't have the time. Later, I usually reread comments, including mine. Sorry if my first reply was misleading!

That being said, and as mentioned above, developers are very welcome to work on pcmanfm-qt, libfm-qt and other components of LXQt.

@micrococo
Copy link
Contributor Author

@tsujan no problem. I didn't know about the state of qtermwidget and the reasons you have stated are enough to discard my request. Thanks.

@tsujan
Copy link
Member

tsujan commented May 26, 2018

I didn't know about the state of qtermwidget

Its state in one word: terrible ;) It still works but how long it'll work if no one attends to it, I don't know.

@agaida agaida added this to test in Issues Aug 11, 2018
@agaida agaida added this to Done in Issues Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues
  
Translations and L10N
Issues
  
Done
Development

No branches or pull requests

3 participants