panel: Add outputs option for per-output configuration#356
panel: Add outputs option for per-output configuration#356
Conversation
soreau
force-pushed
the
panel-outputs
branch
from
3ebfb21 to
1c42542
Compare
soreau
force-pushed
the
panel-outputs
branch
from
1c42542 to
d7bdf58
Compare
| public: | ||
| std::map<WayfireOutput*, std::unique_ptr<WayfirePanel>> panels; | ||
| std::map<std::string, WayfireOutput*> outputs; | ||
| WfOption<std::string> *panel_outputs; |
There was a problem hiding this comment.
Having this dynamically allocated seems really weird, if you need to load an option at runtime, you can use .load_option(<option name>)
There was a problem hiding this comment.
Sounds like this might work.
| * because an '*' character was found in the panel_outputs string */ | ||
| if (std::string(*priv->panel_outputs).find("*") != std::string::npos) | ||
| { | ||
| for (auto& o : priv->outputs) |
There was a problem hiding this comment.
Why do we need a separate map from string to a WayfireOutput ? If we want to iterate over all outputs, I believe gdk has a function for this (https://docs.gtk.org/gdk4/method.Display.get_monitors.html)
There was a problem hiding this comment.
Yes, you need the WayfireOutput to create the panel. You cannot get this from the gdk function, unless you make the wf-shell-app monitors list public, as far as I understand.
There was a problem hiding this comment.
Sounds like we should add a getter then. The less overall state, the better IMO. Considering we already keep it.
There was a problem hiding this comment.
A getter would incur looping each time we call the function and would require modifying code outside of panel.cpp, if I am thinking about this right. A std::map is more efficient here and keeps the patch clean.
| return; | ||
| } | ||
|
|
||
| /* Second case: Add a panel for each output in the panel_outputs string, |
There was a problem hiding this comment.
I believe a simpler algorithm would be like this in pseudocode:
for_each_existing_panel:
if output not allowed in config:
remove panel
for_each_gdk_monitor:
if output allowed in config and does not have panel:
create panel
There was a problem hiding this comment.
This way I believe you won't need any of the new maps you have added, and I have found that generally the less state you have, the less possibilities for things to go wrong.
There was a problem hiding this comment.
I think the pseudo code you have is flawed simply because we do not want to unnecessarily recreate panels, since that will reorder the window-list and reset other widgets.
There was a problem hiding this comment.
I don't see how it is flawed, because I say to create a panel if it does NOT exist already.
There was a problem hiding this comment.
Well it's not very different from what I have so far, I'm sure there are many other ways to skin this cat though.
| new WayfirePanel(output)); | ||
| priv->panel_outputs = new WfOption<std::string>{"panel/outputs"}; | ||
| priv->outputs[output->monitor->get_connector()] = output; | ||
| if ((std::string(*priv->panel_outputs).find("*") != std::string::npos) || |
There was a problem hiding this comment.
We have these checks everywhere, I think it would make sense to add a function is_panel_enabled_in_config(config_opt, output_name)
There was a problem hiding this comment.
That might be doable, but it's really only two cases where we'd need it, if I am thinking correctly.
This adds a new option named 'outputs' that is a comma separated list of output names on which a panel instance should be rendered. The default value is '*' wildcard, which means all outputs.
soreau
force-pushed
the
panel-outputs
branch
from
d7bdf58 to
062b869
Compare
2 participants
This adds a new option named 'outputs' that is a list of output names on which a panel instance should be rendered. The default value is '*' wildcard, which means all outputs.