Skip to content

panel: Add outputs option for per-output configuration #356

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.

Sign up for GitHub

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

Open
soreau wants to merge 1 commit into
base: master
Choose a base branch
from
Open

panel: Add outputs option for per-output configuration #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions metadata/panel.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
<_short>Autohide Duration</_short>
<default>300</default>
</option>
<option name="outputs" type="string">
<_short>Outputs on which to show a panel instance</_short>
<_long>A comma separated list of output names on which to show panel instances. Set to * wildcard for all outputs.</_long>
<default>*</default>
</option>
<option name="position" type="string">
<_short>Panel Position</_short>
<default>top</default>
Expand Down
99 changes: 97 additions & 2 deletions src/panel/panel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,8 @@ class WayfirePanelApp::impl
{
public:
std::map<WayfireOutput*, std::unique_ptr<WayfirePanel>> panels;
std::map<std::string, WayfireOutput*> outputs;
WfOption<std::string> *panel_outputs;
Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this dynamically allocated seems really weird, if you need to load an option at runtime, you can use .load_option(<option name>)

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this might work.

};

void WayfirePanelApp::on_config_reload()
Expand All @@ -425,6 +427,91 @@ void WayfirePanelApp::on_activate()
{
WayfireShellApp::on_activate();

priv->panel_outputs->set_callback([=] ()
{
/* First case: Add a panel for each output that does not have one,
* 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)
Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we should add a getter then. The less overall state, the better IMO. Considering we already keep it.

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
bool panel_exists = false;
for (auto& p : priv->panels)
{
if (p.first->monitor->get_connector() == o.first)
{
panel_exists = true;
break;
}
}

if (!panel_exists)
{
std::cout << "Adding panel for output: " << o.first << std::endl;
priv->panels[o.second] = std::unique_ptr<WayfirePanel>(
new WayfirePanel(o.second));

priv->panels[o.second]->handle_config_reload();
priv->panels[o.second]->set_panel_app(this);
priv->panels[o.second]->init_widgets();
}
}

return;
}

/* Second case: Add a panel for each output in the panel_outputs string,
Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how it is flawed, because I say to create a panel if it does NOT exist already.

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* removing panels that are not found in the string */
for (auto& o : priv->outputs)
{
bool panel_exists = false;
for (auto& p : priv->panels)
{
if (p.first->monitor->get_connector() == o.first)
{
panel_exists = true;
break;
}
}

if (std::string(*priv->panel_outputs).find(o.first) != std::string::npos)
{
if (!panel_exists)
{
std::cout << "Adding panel for output: " << o.first << std::endl;
priv->panels[o.second] = std::unique_ptr<WayfirePanel>(
new WayfirePanel(o.second));

priv->panels[o.second]->handle_config_reload();
priv->panels[o.second]->set_panel_app(this);
priv->panels[o.second]->init_widgets();
}
} else
{
std::cout << "Removing panel from output: " << o.first << std::endl;
priv->panels.erase(o.second);
}
}
});

if (priv->panels.empty())
{
std::cout << std::endl <<
"WARNING: wf-panel outputs option did not match any outputs, " \
"so none were created. Set the [panel] outputs option to * " \
"wildcard character in wf-shell configuariton file to match " \
"all outputs, or set one or more of the following detected outputs:" <<
std::endl << std::endl;

for (auto& o : priv->outputs)
{
std::cout << o.first << std::endl;
}

std::cout << std::endl << "Currently the [panel] outputs option is set to: " <<
std::string(*priv->panel_outputs) << std::endl;
}

const static std::vector<std::pair<std::string, std::string>> icon_sizes_args =
{
{"panel/minimal_height", ""},
Expand Down Expand Up @@ -466,8 +553,15 @@ std::shared_ptr<WayfireIPC> WayfirePanelApp::get_ipc_server_instance()

void WayfirePanelApp::handle_new_output(WayfireOutput *output)
{
priv->panels[output] = std::unique_ptr<WayfirePanel>(
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) ||
Copy link
Member

@ammen99 ammen99 Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have these checks everywhere, I think it would make sense to add a function is_panel_enabled_in_config(config_opt, output_name)

Copy link
Member Author

@soreau soreau Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be doable, but it's really only two cases where we'd need it, if I am thinking correctly.

(std::string(*priv->panel_outputs).find(output->monitor->get_connector()) != std::string::npos))
{
std::cout << "Adding panel for output: " << output->monitor->get_connector() << std::endl;
priv->panels[output] = std::unique_ptr<WayfirePanel>(
new WayfirePanel(output));
}
}

WayfirePanel*WayfirePanelApp::panel_for_wl_output(wl_output *output)
Expand All @@ -486,6 +580,7 @@ WayfirePanel*WayfirePanelApp::panel_for_wl_output(wl_output *output)
void WayfirePanelApp::handle_output_removed(WayfireOutput *output)
{
priv->panels.erase(output);
priv->outputs.erase(output->monitor->get_connector());
}

WayfirePanelApp& WayfirePanelApp::get()
Expand Down