Clicking on menu item selects the sensor shown in panel.

[farsx]

PopupMenuItems of sensors are now reactive. Clicking on one of them change the setting for the sensor to display in panel.

[adrianbroher]

Hello Alessandro,

To be honest I don't know if this is a good change. I currently see the following issues with this off the top of my head:

• The user could unintentionally select a sensor and wouldn't be immediately aware of the change whereas the selection over the preferences dialog is an explicit action.
• There is no feedback what is the currently selected sensor. Granted, this isn't neither the case with the current solution.
• There is no way to select the average and maximum temperature. When there is a way to change a setting in different places the available should be the same.

Any opinion on that thoughts?

[farsx]

Hi Marcel,

for the first two points I agree with you. Infact, I thought to add an option to add also the sensor label to the panel, displaying for example "Core 0: 40C". Perhaps we could enslave the 'reactiveness' of the menu items to that option. Alternatively we could add a dot near the selected menu item (like in cpu-freq shell extension).

About average and maximum temperature I understand the settings aren't uniform but I don't believe that's a great issue. We could also display them in the list, this would be a better approach if we think about 'uniformity'.

Summarizing, I think that it could work well if we display also average and maximum temp and add a dot next to the selected menu item.

Let me know your thoughts... and sorry for the long reply! :)

[adrianbroher]

Infact, I thought to add an option to add also the sensor label to the panel, displaying for example "Core 0: 40C".

This sounds independent of this issue like a good idea to me. If you provide a pull request for that I would add that.

Alternatively we could add a dot near the selected menu item (like in cpu-freq shell extension). Could you point out an example screenshot of cpu-freq for reference or upload one that shows this?

But I see a problem there: I tried to add icons in front for the different types of sensors and the 'Sensors Settings' entry doesn't span over serveral columns pushing the Labels to the right and adding an ugly spacing to the menu. I haven't found a solution for that yet.

We could also display them in the list, this would be a better approach if we think about 'uniformity'.

No objection for the average temperature. However for the top temperature you would have a duplicate entry. But this could be solved with having a separator separating the calculated special sensor data from the actual sensors.

Summarizing, I think that it could work well if we display also average and maximum temp and add a dot next to the selected menu item.

I forgot to ask, but do you plan to drop the drop down menu in favour of that or should be an additional way to configure the main sensor?

[farsx]

This sounds independent of this issue like a good idea to me. If you provide a pull request for that I would add that.

Ok, I'll work on this! It shouldn't be difficult.

Could you point out an example screenshot of cpu-freq for reference or upload one that shows this?

Here is the screenshot. As you can see it's not of cpu-freq but it's exactly what I meant.

But I see a problem there: I tried to add icons in front for the different types of sensors and the 'Sensors Settings' entry doesn't span over serveral columns pushing the Labels to the right and adding an ugly spacing to the menu. I haven't found a solution for that yet.

I'm not sure I understood. Does the screenshot remove your concern?

No objection for the average temperature. However for the top temperature you would have a duplicate entry. But this could be solved with having a separator separating the calculated special sensor data from the actual sensors.

Yes, I was thinking of using a separator and put Maximum and Average Temperature just above Settings entry.

I forgot to ask, but do you plan to drop the drop down menu in favour of that or should be an additional way to > configure the main sensor?

No, I believe that the preferences window should be complete. That would be a quick way to select the main sensor.

[farsx]

Ah sorry I closed the issue by mistake...

[adrianbroher]

I'm not sure I understood. Does the screenshot remove your concern?

Not really. Here a picture for clarification.

[farsx]

Ah OK now I understand. Btw... It's really super cool! :smile: Forget about the dot, it would mess things up... This is far better. We can just use bold to show the main sensor.

About your issue, can't you move 'Sensors Settings' to the second column? Or just use an empty icon for that line?

[farsx]

Hi Marcel, I modified the code as discussed, adding Average and Maximum temperature as menu item and highlighting the main sensor (the one shown in panel) in bold. I also simplified the settings schema, removing useless 'show-in-panel' entry.

[adrianbroher]

It's really super cool!

Thanks

Forget about the dot, it would mess things up... This is far better. We can just use bold to show the main sensor.

Another Idea would be to create an inverted icon for that. But well, that's nothing important for now.

About your issue, can't you move 'Sensors Settings' to the second column? Or just use an empty icon for that line?

That's a hacky solution and I don't like those. Especially if there is already the 'span' parameter that doesn't work for some reason.

I modified the code as discussed, adding Average and Maximum temperature as menu item and highlighting the main sensor (the one shown in panel) in bold.

That looks a lot better than the initial suggestion so I merged it. Thanks for your contribution.