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

PCP: DynamicScreens #1102

Closed
wants to merge 10 commits into from
Closed

PCP: DynamicScreens #1102

wants to merge 10 commits into from

Conversation

smalinux
Copy link
Contributor

@smalinux smalinux commented Oct 6, 2022

No description provided.

Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

I think some places that include config.h don't need it included. Please re-check.

CommandLine.c Outdated Show resolved Hide resolved
Action.c Show resolved Hide resolved
DynamicColumn.h Outdated Show resolved Hide resolved
DynamicScreen.c Outdated Show resolved Hide resolved
DynamicScreen.c Outdated Show resolved Hide resolved
pcp/PCPGenericData.h Outdated Show resolved Hide resolved
pcp/PCPGenericDataList.c Outdated Show resolved Hide resolved
pcp/PCPGenericDataList.c Outdated Show resolved Hide resolved
pcp/PCPGenericDataList.c Outdated Show resolved Hide resolved
pcp/PCPGenericDataList.c Outdated Show resolved Hide resolved
@natoscott
Copy link
Member

@smalinux I tried building with --enable-debug and am now seeing an assert failure when I move to the first BPF tab (Opensnoop) - any ideas? It looks like the Vector_add code is checking for something that has type Process_class (and these BPF entries are not that) ...

#0  0x00007ffff7a8ec4c in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff7a3e9c6 in raise () from /lib64/libc.so.6
#2  0x00007ffff7a287f4 in abort () from /lib64/libc.so.6
#3  0x00007ffff7a2871b in __assert_fail_base.cold () from /lib64/libc.so.6
#4  0x00007ffff7a37576 in __assert_fail () from /lib64/libc.so.6
#5  0x000000000041f348 in Vector_set (this=0x713000, idx=idx@entry=0, data_=0x79ad30)
    at Vector.c:339
#6  0x00000000004134de in Panel_set (this=<optimized out>, i=i@entry=0, o=<optimized out>)
    at Panel.c:135
#7  0x000000000040c8f4 in GenericDataList_rebuildPanel (this=this@entry=0x6b08c0)
    at GenericDataList.c:85
#8  0x000000000041a07b in checkRecalculation [...]
(gdb) up 5
#5  0x000000000041f348 in Vector_set (this=0x713000, idx=idx@entry=0, data_=0x79ad30)
    at Vector.c:339
339	   assert(Object_isA(data, this->type));
(gdb) p this->type
$3 = (const ObjectClass *) 0x431700 <Process_class>

Copy link
Member

@natoscott natoscott left a comment

Choose a reason for hiding this comment

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

Looking good @smalinux - taking htop to another level.

Do you have a list of known issues? Here's a start (no surprises here since we've chatted about these, just trying to track 'em so we can tick everything off before any merge) ...

  • scaling of values needs to be more dynamic, drop from 'scale' config files
  • the Available Columns from Setup should adjust per screen
  • filesystem screen is a little confusing
  • add a biotop screen as well

MainPanel_setState(panel, &state);
//MainPanel_setState(panel, &state);
panel->state = &state;
genericDataPanel->state = &state;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ... ?

   MainPanel_setState(panel, &state);
   MainPanel_setState(genericDataPanel, &state);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@natoscott I think we need a new panel working in parallel with/instead MainPanel. This is a big part of what I'm talking about so far...
this panel will be minimal and doesn't have Nice (f7) or Kill (f9) panels [dito]
This will fix a lot of issues, maybe one of them is the issue mentioned above --enable-debug, not sure, this part needs surgery ^^"
but MainPanel has Class(Process):

Panel_init((Panel*) this, 1, 1, 1, 1, Class(Process), false, FunctionBar_new(Settings_isReadonly() ? MainFunctions_ro : MainFunctions, NULL, NULL));

also, @BenBE told me before how to do that: @natoscott I need your help here...

BenBe: I think it might be worth looking into a direction where the current Panel code is just used as a derived class ProcessListPanel (showing a process list), while most of its code is split into a base class GenericPanel (doing mostly nothing). From that GenericPanel class you could then split another derived class "FancyPanel" for doing all the Fancy stuff ;-)

Sohaib: Could you explain this more please?

BenBe: Basically what I was going at was not adopting the current Panel class to also show generic stuff, but to use the existing inheritance stuff so you can just push different kinds of Panels ("Process Lists" vs. "Generic Panels") into the list of tabs handled by the ScreenManager code.
The way to do this mostly boils down to split the current Panel implementation into the basic "draw the panel" code (extracted from current source BasePanel class) and the code drawing the process list (a new ProcessListPanel class).
The ScreenManager then instead of listing plain Panels would list BasePanels, which could be ProcessListPanels (working like they do now) or GenricStuffPanels (doing something generic).

Copy link
Member

Choose a reason for hiding this comment

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

@BenBE any chance you can help @smalinux here with some sample code changes to get him started on this aspect? thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Will have to wait for next week; not in town tomorrow and the day after (giving a talk for a small conference). Changes-wise it boils down to creating inherited classes similar to what can be seen with ProcessList and <Platform>ProcessList with the Panel managing the ProcessList stuff getting most of the current process list draw code. Extra points when rendering the header/list items is split in a way so that for the generic tabs and the process list the actual rendering code is the same, but only creating/filling the content buffers differs. As said, I can elaborate next week …

Copy link
Member

Choose a reason for hiding this comment

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

@BenBE ping - can you post that detailed writeup here when you get a free minute? :) Thanks!

pcp/screens/cgroups Outdated Show resolved Hide resolved
pcp/screens/filesys Outdated Show resolved Hide resolved
pcp/screens/filesys Outdated Show resolved Hide resolved
@smalinux smalinux force-pushed the DynamicScreens branch 4 times, most recently from e4f8e11 to ba84d96 Compare October 9, 2022 06:44
@smalinux smalinux force-pushed the DynamicScreens branch 4 times, most recently from b6f0901 to 586ced4 Compare October 26, 2022 16:55
@BenBE
Copy link
Member

BenBE commented Oct 26, 2022

@smalinux The rename changes from your second commit are partially included in the first commit (e.g. look for pauseUpdate in the first commit). I think putting that refactoring commit in front and ensuring the changes there are complete is most clear to get things work out correctly. A quick test is to try and compile each commit individually and see if it builds properly.

natoscott and others added 9 commits February 14, 2023 11:51
'actionTogglePauseProcessUpdate' -> 'actionTogglePauseUpdate'
'pauseProcessUpdate' -> 'pauseUpdate'
'hideProcessSelection' -> 'hideSelection'
'hideProcessSelection' -> 'hideSelection'

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
Adds support for new 'format' keyword, updates to screens config files,
and some minor cleanup here & there.
@natoscott
Copy link
Member

This functionality has now been incorporated into PR #1243 (after fairly substantial reworking).

@natoscott natoscott closed this Jun 19, 2023
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.

3 participants