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

Logic for improved guake like terminal functionality #579

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maximstewart
Copy link

@maximstewart maximstewart commented Feb 9, 2022

I wanted to add an improved Guake like terminal experience that adds options to lock the window to the top, bottom, left, or right part of any monitor. In addition, I wanted to override the toggle button so that the user could bind multiple terminals to separate toggle keys. This is on par with what Stjerm does which is abandoned at this time.

Edit: Fixed a stupid mistake after code cleanup.

Edit: I found the request I made some time back:
#298

Yes, it was from an old account that no longer exists. Anyway, this is intended to address the above closed issue/feature request.

@maximstewart maximstewart force-pushed the feature/guake-like-functionality branch from d3c2f60 to e1fe0a7 Compare February 9, 2022 04:41
@mattrose
Copy link
Member

I really like this PR, and I'll look at it as soon as I can, but I'm kinda busy right now, so I'm not sure when I'll be able to look at it, but hopefully within the next week or so.

Thanks for contributing!

@maximstewart
Copy link
Author

@mattrose Glad you like the PR! I hope you like my code just as much. I'm happy to make any changes as requested to the best of my ability. Please note that I've been dog fooding my change and it all seems to work as expected with one exception. I've found a weird bug where in fullscreen mode the terminal when bound to the right will move to the 2nd monitor even if the mouse is on the first. I'm still considering potential fixes for that.

Also, my environment is using Openbox with Xcompmgr. I don't believe that matters but I mention just in case anyone sees other issues in different setups.

@maximstewart maximstewart force-pushed the feature/guake-like-functionality branch from e1fe0a7 to 0ad4a1f Compare February 13, 2022 01:52
@maximstewart
Copy link
Author

I did find another weird bug where terminator itself couldn't seem to launch a new instance with the guake width and height arguments because it itself passed a string instead of int type. I changed it to default to string and then do a conversion to int when pulling the guake width and height. Anyway, I believe I've resolved all known issues and pushed a clean and rebased commit.

@maximstewart
Copy link
Author

Just wanted to update that everything has been stable for me using this "patch" or feature. I've not found any other bugs or issues hat needed my addressing that I haven't already done. Obviously this is just on my system but I don't believe it will be different elsewhere.

@mattrose
Copy link
Member

Sorry, this is still on my radar, I just haven't had time to look at it yet. I hope tomorrow I'll have a little bit of free time after doing housework

@mattrose
Copy link
Member

Just looking at this now. I would much rather have config file options and preferences set, rather than using command-line parameters. If you could just add config file entries for that, and a way to enter the preferences via the preferences UI, that would be appreciated. I'll take a look at it, but working with the preferences UI is, honestly, not my favourite.

@mattrose
Copy link
Member

I puilled in your code, and I couldn't get it to work. How are you calling terminator? can you paste in a command that works for you?

@maximstewart
Copy link
Author

maximstewart commented Mar 29, 2022

@mattrose

Here is a start command that I often use:

terminator --guake-key="F8" --guake-side left --guake-width 700 --guake-height 800

To address your other points:

Editing a config file forces you to open an editor and make changes. Having it this way allows you to quickly spawn an instance as needed. Adding it to the UI would seem to pose a similar issue in my mind. In addition, the main way I use this is through my start script for my system. If I add a config entry then it would seem I'd start an terminator instance just to start another terminator instance. Though, I could be misunderstanding your intent.

@mattrose
Copy link
Member

If you set those options in the config file, they could be run every time you started up terminator, and then you would just run terminator in your startup script, rather terminator --guake-key="F8" --guake-side left --guake-width 700 --guake-height 800, and it would work just as well.

That command you send works but if you're really attached to it being command-line params it'll need documentation in the help text and the man page, preferably with the quake-key having an example.

I do like the idea, but I think you could clean up the code a lot by using config variables rather than command-line options, and still have it work the way you want.

@maximstewart
Copy link
Author

@mattrose

I'm not familiar with how terminator config is structured; but, wouldn't setting this in the config mean that each time you run terminator it spawns new instances of the same pinned terminals? If so, I most certainly want to keep it as cmd args.

@mattrose
Copy link
Member

mattrose commented Apr 1, 2022

I don''t think so. terminator only uses one process, and just spawns new windows upon repeated calls of terminator (unless you specifically disable dbus). I will play with it some and get back to you.

@maximstewart
Copy link
Author

maximstewart commented Apr 5, 2022

Sounds good, I'll await your findings. But, to ask and address some other points, by adding this to the config variables I would be limited to binding 4 windows, correct? Also, to be frank, I'm unsure of what help text file and man page files to edit. Still working out the project structure a bit. Also, I'm looking into adding x and y offsets as an option too.

@mattrose
Copy link
Member

Hmm, this doesn't seem to work on my F34 desktop with wayland. Does this require X11 to work?

@maximstewart
Copy link
Author

@mattrose Based on the import section at line 29 It is. But, Keybinder could be fixed to work with Wayland at this point. I didn't really look too deep.

@maximstewart
Copy link
Author

I hope you are doing well. I am just checking in to see if you have done further testing?

@maximstewart maximstewart force-pushed the feature/guake-like-functionality branch from 0ad4a1f to 1225d87 Compare January 22, 2023 22:46
@maximstewart
Copy link
Author

So, sorry for taking so long to get back to this but I added what I think are the needed changes for the man page. I git rebased my commit and added changes to doc/terminator.1. I also did do some slight code cleanup/variable rename as well but nothing major. In addition, I updated some verbiage for terminatorlib/optionparse.py and added an import for RawTextHelpFormatter which allows the argparse add_argument method to format help text as expected when using \n. (I can remove if not desired but otherwise the example text is long and on the same line as the description.) I found that fix the following SA post:

https://stackoverflow.com/questions/3853722/how-to-insert-newlines-on-argparse-help-text

@maximstewart maximstewart reopened this Sep 19, 2023
@maximstewart
Copy link
Author

Ngl, I almost gave up on this given the misery I had trying to pull new changes, resolve merge conflict, then totally bungle it up, and thereafter try reverting my bungled merge only to revert my work but still pull in the newer changes. I got lucky that github shows the commits even if not tied to a branch.

@maximstewart maximstewart force-pushed the feature/guake-like-functionality branch from 8d4b661 to 9248fc9 Compare February 18, 2024 16:55
@mattrose
Copy link
Member

No problem. I'll pull down your changes soon and take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants