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

Rewrite minimization-related modules #102

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Conversation

f1u77y
Copy link
Contributor

@f1u77y f1u77y commented Oct 25, 2016

  • Use global state instead of per-layout - so now window is minimized on
    all workspaces (EWMH requires that windows with _NET_WM_STATE_HIDDEN
    set should be minimized on any workspace but previously they were not)
  • Use windows instead of modify. That should fix bugs related to
    actions that should be done by windows and not done by
    modify (fixes multiple windows are highlighted as active when using X.L.Minimize #46)
  • Deprecated module X.H.RestoreMinimized is removed

TODO 515429f should be moved to another PR

UPD TODO entry in CHANGES.md

@byorgey
Copy link
Member

byorgey commented Nov 3, 2016

I think another PR containing something similar to @515429ff has now been merged, so this now has a conflict.

@f1u77y f1u77y force-pushed the rewrite-minimize branch 2 times, most recently from 22f72f7 to d4f4f0d Compare November 3, 2016 21:51
@colonelpanic8
Copy link
Contributor

What's the status of this PR?

@colonelpanic8
Copy link
Contributor

@byorgey
Is there anything blocking a merge here?

@f1u77y
Copy link
Contributor Author

f1u77y commented Nov 29, 2016

@IvanMalison this PR should leave the old behaviour as is(as geekosaur said), so yes

@colonelpanic8
Copy link
Contributor

@f1u77y are you planning to make the changes needed to get this merged?

This should get merged before .13 right?

@pjones
Copy link
Contributor

pjones commented Feb 7, 2017

@f1u77y What is the status of this PR? It looks like it no longer contains any conflicts.

The only problem I see is that this PR deletes X.H.RestoreMinimized. I think we should actually add some {-# DEPRECATED #-} pragmas first, before we git rid of it.

@pjones pjones modified the milestones: v0.13, v0.14 Feb 7, 2017
@f1u77y f1u77y force-pushed the rewrite-minimize branch 2 times, most recently from 2036556 to c70759b Compare February 12, 2017 07:23
@f1u77y
Copy link
Contributor Author

f1u77y commented Feb 12, 2017 via email

@geekosaur
Copy link
Contributor

How many people read docs, especially after they have something working? The pragma means they get a warning every time they recompile their config.

@colonelpanic8
Copy link
Contributor

IS there anything blocking merge here?

@pjones
Copy link
Contributor

pjones commented Feb 21, 2017

Someone needs to review the code and test it with their config. So it's just a matter of when a core team member having the time to do so. This issue is part of the v0.14 milestone which I'm hoping will be released in Q2 of this year.

@pjones
Copy link
Contributor

pjones commented Apr 10, 2017

@f1u77y Please add an entry to CHANGES.md so we can get this merged. Thanks.

@colonelpanic8
Copy link
Contributor

I'd be happy to add to the CHANGES.md.

Lets get this merged

@colonelpanic8
Copy link
Contributor

colonelpanic8 commented Apr 14, 2017

@pjones
Copy link
Contributor

pjones commented Apr 14, 2017

@f1u77y Can you merge in the changes from @IvanMalison, and then rebase and squash commits? Thanks.

@colonelpanic8
Copy link
Contributor

@f1u77y This still needs a squash

@colonelpanic8
Copy link
Contributor

colonelpanic8 commented Apr 17, 2017

@pjones Actually, because of the way the merges were done (I realize that this is against the contributing guidelines) a squash would be somewhat difficult. It would be possible to go back and cherry-pick all of the relevant commits on top of the current HEAD of contrib if you would prefer that. I think it might be better to just leave the history as is.

@f1u77y
Copy link
Contributor Author

f1u77y commented Apr 20, 2017

@pjones @IvanMalison I've squashed the commits. This is the old version of the branch. I hope I haven't missed something. At least that works well on my config.

Sorry for the delay.

@pjones
Copy link
Contributor

pjones commented Apr 20, 2017

@f1u77y Awesome! Thanks for doing that.

Last question. Since I don't use this module, what can users expect upon upgrade? Will their configuration files still compile and work?

@f1u77y
Copy link
Contributor Author

f1u77y commented Apr 20, 2017

No, because old configuration uses sending messages to layout and new one uses functions instead. I think the docs are descriptive enough to let user migrate easily.

EDIT Another way is leave old modules as is and rename the new modules, so users won't be left with non-working configs but I think it's a little messy.

@pjones
Copy link
Contributor

pjones commented Apr 20, 2017

I think it's okay to break things. But, please add an entry to the change log in the "breaking changes" section that makes it clear that config files need to be updated after the upgrade. Thanks.

@f1u77y
Copy link
Contributor Author

f1u77y commented Apr 20, 2017

@pjones done. If there are no other questions I'll squash it.

* Use global state instead of per-layout - so now window is minimized on
  all workspaces (EWMH requires that windows with _NET_WM_STATE_HIDDEN
  set should be minimized on any workspace but previously they were not)
* Use `windows` instead of `modify`. That should fix bugs related to
  actions that should be done by `windows` and not done by
  `modify` (fixes xmonad#46)
* Mark module X.H.RestoreMinimized as deprecated
@pjones
Copy link
Contributor

pjones commented Apr 20, 2017

Looks good. After you squash please rebase on master. The bit in the change log about the manage hook was removed in a recent commit.

@f1u77y
Copy link
Contributor Author

f1u77y commented Apr 20, 2017

I don't understand what bit are you talking about(is it c99606b#diff-8b1c3fd0d4a6765c16dfd18509182f9dL78 ?). This branch is up-to-date with upstream/master, so rebasing has no effect.

@pjones
Copy link
Contributor

pjones commented Apr 20, 2017

I'm talking about the "ewmh function from X.H.EwmhDesktops" text in CHANGES.md. It must have not been removed when I reverted another commit earlier. I'll take care of it.

Thanks for all the work on this.

@pjones pjones merged commit 7e47ecc into xmonad:master Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple windows are highlighted as active when using X.L.Minimize
5 participants