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

Refactoring, positioning fix and pushing towards #364 #365

Merged
merged 12 commits into from
Sep 4, 2014

Conversation

schnittchen
Copy link
Contributor

I did only a very superficial test and it "works for me".

I can set the display_n setting to -1 (using gconf-editor) and have the window appear on my primary monitor.

@gsemet
Copy link
Member

gsemet commented Aug 26, 2014

I don't understand your question.

I agree on the patch, however "-1" should be available in the preference window.

@schnittchen
Copy link
Contributor Author

Next thing would be to use a constant instead of just -1, for readibility.

I doubt I can manage the necessary changes in the front-end, having no gtk experience at all :(

@schnittchen
Copy link
Contributor Author

Update: prefs UI update upcoming.

@schnittchen
Copy link
Contributor Author

Is 6dd227b a bad idea?
It removes a gconf key that is still referenced by some comments, but not by any code.

When the configured monitor does not exist, the comment says it falls back to primary_display option, but instead falls back on the mouse option. I wonder if it should now instead fall back to the new "always on primary" option.

@gsemet
Copy link
Member

gsemet commented Aug 27, 2014

I would not force people to use mouse monitor. I kind of like the possibility to choose whether we want to display the terminal on the primary or secondary monitor, and in option on the monitor where the mouse is (and for me it should be the default option).

Idealy the combo box should have the following items:

  • "Monitor where the mouse is"
  • "Primary monitor"
  • "Secondary monitor" (only displayed if more than one"
  • and so on (let say that after monitor 8, we just display "On monitor N".

@gsemet
Copy link
Member

gsemet commented Aug 27, 2014

I also think 6dd227b duplicate a feature, useless now.

@schnittchen
Copy link
Contributor Author

I think X has no notion of secondary monitor. There is monitors 0, 1, 2... and one of them is flagged primary (which is not necessarily 0). This leaves us with n+2 options, n for each monitor, "always on monitor" and "mouse".

Maybe pushing these additional options into the combo box is not such a good idea, because the iter_ stuff is really fiddly.

I believe the code is technically OK now, however, do we really want to encode the new option as display_n == -1?

@schnittchen
Copy link
Contributor Author

ping

@gsemet
Copy link
Member

gsemet commented Sep 4, 2014

I had no time this week to test this quite impact patch, unfortunately :(

gsemet added a commit that referenced this pull request Sep 4, 2014
Refactoring, positioning fix and pushing towards #364
@gsemet gsemet merged commit 876eb1b into Guake:master Sep 4, 2014
@gsemet
Copy link
Member

gsemet commented Sep 4, 2014

I just found one small bug, sometime when we switch the windows and let the terminal opened, it does not display on the expected display

@schnittchen
Copy link
Contributor Author

Thanks for merging.

Does toggling visibility twice make it appear on the right display again? Are all of the display_n options affected?

gsemet added a commit that referenced this pull request Jan 30, 2017
Refactoring, positioning fix and pushing towards #364
gsemet added a commit that referenced this pull request Jan 30, 2017
Refactoring, positioning fix and pushing towards #364
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