Introduce Row and Table classes for tabular screens beyond top-processes

[natoscott]

This commit refactors the Process and ProcessList structures such they each have a new parent - Row and Table, respectively. These new classes handle screen updates relating to anything that could be represented in tabular format, e.g. cgroups, filesystems, etc, without us having to reimplement the display logic repeatedly for each new entity.

[Explorer09]

I didn't read the entire commit, but I have one comment here:
I don't like the class naming "Table" and "Row" here. Considering we have Hashtable class already, the name Table could draw confusion for "what does this Table class do exactly and how is it different from Hashtable".
I can't suggest a better name because I'm not sure of the goals of these classes for now. UITable? UIDataTable? Would these names be better?

[BenBE]

Only checked the platform-independent code so far. Several remarks and annotations.

One open question is distributing a Table's contents onto different Screens while keeping single instances of various table types as needed (n:m linkage).

[natoscott]

[...] Would these names be better?

I don't think so, personally. I've chosen Table and Row to match with the existing use of Column throughout the code base, which is not prefixed in any way to refine/clarify its meaning - it's self evident in the Column case and I think the same is true for Row and Table. These are also the simplest possible names for these things, which is best for a base class IMO.

I didn't read the entire commit, but I have one comment here: [...]
I think I'm still confused about the purpose of the Row class here. [...]

These aspects will be clearer once there's a second user of these classes (final commit after this one, which will be done as soon as time is available - it's an adaptation of #1102 to this though). This PR is not for approving/merging until that arrives, more to get early feedback (thanks!) and to give a heads-up that's (still) progressing.

[...] Make it a composition relation instead.

I agree with @BenBE here, I'm finding its a much better fit to have these as 'inherited' base classes.

I'm juggling a few projects here, context-switching alot, but will get updated code and the final PR in this series open ASAP for more discussion.

[Explorer09]

I don't think so, personally. I've chosen Table and Row to match with the existing use of Column throughout the code base, which is not prefixed in any way to refine/clarify its meaning - it's self evident in the Column case and I think the same is true for Row and Table.

What existing Column code base are you referring to? Because I don't see any Column.c or Column.h files.

These are also the simplest possible names for these things, which is best for a base class IMO.

I disagree when any one can look at the code and ask "why isn't a Hashtable derived from a Table", for example. Even a class name like DataTable can clarify a lot of things that a generic Table name cannot. And this new code is meant to be maintained long term.

These aspects will be clearer once there's a second user of these classes (final commit after this one, which will be done as soon as time is available - it's an adaptation of #1102 to this though). This PR is not for approving/merging until that arrives, more to get early feedback (thanks!) and to give a heads-up that's (still) progressing.

I'll wait and see. But for now, I don't think the current code refactoring make sense.

[Explorer09]

Two more comments:

1. When a Process object can have PID as a primary key that uniquely identifies the object and be use for sorting, I don't know if it make sense to have that id applied to any other kind of things htop can monitor (cgroups, filesystems, etc.) Maybe other objects should use different primary keys. This, I kind of disagree when Row contains an id member and that Process.pid derives from it.
2. The RowField name. Maybe you meant Field instead (or DataField if clarity is needed) so that the Row class can become Record (DataRecord). That's one of the awkward class naming I was complaining about.

[BenBE]

Okay, some more comments for the stuff I didn't look at previously. Mostly consistency fixes, nothing too special. Overlaps in big parts with things already noted for the platform-independent stuff.

Not too happy yet to have tgpid (group) and uid as part of the Row class …

[BenBE]

I don't think so, personally. I've chosen Table and Row to match with the existing use of Column throughout the code base, which is not prefixed in any way to refine/clarify its meaning - it's self evident in the Column case and I think the same is true for Row and Table.

What existing Column code base are you referring to? Because I don't see any Column.c or Column.h files.

Currently called ProcessFields (i.e. IDs for values we display) which are amended by the DynamicColumns (i.e. values+names handled only at runtime dynamically [mostly for PCP]).

These are also the simplest possible names for these things, which is best for a base class IMO.

I disagree when any one can look at the code and ask "why isn't a Hashtable derived from a Table", for example. Even a class name like DataTable can clarify a lot of things that a generic Table name cannot. And this new code is meant to be maintained long term.

Hashtable vs. Hashmap have historically been used interchangeably for these kind of data structures, which Python calls dict(ionaries).

Re Table vs. DataTable: I can cope with both. And given the kind of Database analogy we have in the background (we are working on sets of resources that have relations), it's not too far off to borrow the naming from there. After all, the largest part of this PR is introducing an additional abstraction, which was not needed previously as htop only managed simple lists of resources (processes). With several ideas in the pipeline that need to display different kinds of resources, this abstraction becomes relevant enough to warrant splitting functionality and avoid the code duplication which would be the alternative.

These aspects will be clearer once there's a second user of these classes (final commit after this one, which will be done as soon as time is available - it's an adaptation of #1102 to this though). This PR is not for approving/merging until that arrives, more to get early feedback (thanks!) and to give a heads-up that's (still) progressing.

I'll wait and see. But for now, I don't think the current code refactoring make sense.

I know what @natoscott is going for as I helped with the GSoC project last year were most of that code was created. The refactoring now is basically the aftermath of all the cleanup not done back then. Having a look at #1102 you can get a glimpse of why the current internal structure is bad for handling things that are mostly determined at runtime.

Another use (apart from the immediate PR) can be seen for things like e.g. listing open files (long term project incoming from my side, cf. lsof-org/lsof#268) live, where the current implementation is basically wrapping a static snapshot onto a list of lines to display. With this abstracted Table infrastructure in place (and the WIP work at lsof) we can get live updates for the list of open files without causing headaches all over the place.

Oh, and what about views like the list of network interfaces, their IPs, traffic stats and so on … Also stuff that benefits from having the basic infrastructure for displaying arbitrary data as a table in place (and network interfaces even have a hierarchical structure based on bonding, VLANs, bridges, …).

[natoscott]

| What existing Column code base are you referring to? Because I don't see any Column.c or Column.h files.

Ah that's a literal interpretation of what I meant - sorry, wasn't clear - what I more meant was we don't see naming like "ColumnUI" - there's lots of things that just talk just about Columns (in various contexts).

Your suggestion of "Record" may well be a better name than "Row" - it was one of the options considered earlier. Row is shorter, which is goodness, and "Record" is slightly ambiguous in that it also has "saving data to disk" connotations. Naming is hard.

In terms of the "not liking" the refactoring, maybe it will help to forget about the PCP use case and think more from the perspective of the cgroups discussion in #1191 - i.e. how would we need to refactor the code so that cgroups could be shown in the way processes are today (with tree-view as well, etc)? We definitely need to do exactly the sorts of things being done in this PR.

This is how we've ended up with this refactoring - we have the initial PCP implementation that doesn't do this (ends up duplicating lots of Process.c/ProcessList.c code) and doesn't help the other htop platforms to move forward and allow for other types of Screens in the future. Hence, we're heading this way to try prepare the way for other platforms that might want to do this kind of thing too.

[Explorer09]

@natoscott I'm okay when you have something in mind when you refactor the code with the new classes. I just didn't know how would, e.g, cgroups be based on these new classes, and thus missed the goal you have for this refactoring.

Can you provide us a link to the ”initial PCP implementation” you are talking about, just to get a clearer vision on how the base class Row can be structured?

Your suggestion of "Record" may well be a better name than "Row" - it was one of the options considered earlier. Row is shorter, which is goodness, and "Record" is slightly ambiguous in that it also has "saving data to disk" connotations. Naming is hard.

I was also considering the name like DataEntry or maybe even Node (the term "node" makes sense in the context of tree data structures). But I think it's better to forget the naming for now, and focus on finishing the code structure. A good class name may come out later naturally when things are almost finished.

[natoscott]

An example Filesystem dynamic screen.

[natoscott]

An example of what the Screens setup page looks like with dynamic screens.

[Explorer09]

@natoscott The dynamic screen feature for PCP looks interesting. Before I try to review the code, I have one comment on the Setup screen UI:

The "Available" list (list of available screens) may be merged with the "Available columns" list (list of available columns of that screen). This would save some horizontal space as there are too many columns on that particular Setup screen.

[natoscott]

The "Available" list (list of available screens) may be merged with the "Available columns" list (list of available columns of that screen). This would save some horizontal space as there are too many columns on that particular Setup screen.

We would like to keep the existing htop functionality of allowing the user to choose the displayed screen name (i.e. the string shown in the tab above each Screen). Ultimately, this means these two columns need to be separate - we need a list of (not editable) screen names (i.e. 'Processes' from htop core, followed by the list of dynamic screens from configuration files) and a list of (editable) names that the user selects - the latter is based on the former, but they are different things.

In the example Setup screenshot I posted, we can see "Main" and "I/O" are two differently-configured "Processes" Screens. These names can be edited as the user wishes, but since we now have more than just Processes, a separate (non-editable) column is needed to indicate what the underlying Screen is.

In a similar way to Main and I/O, we can also have zero or more of each of the dynamic screens. So we could have users with multiple different Filesystem tab configurations, for example, each with different display names and different visible columns.

[Explorer09]

@natoscott

In the example Setup screenshot I posted, we can see "Main" and "I/O" are two differently-configured "Processes" Screens. These names can be edited as the user wishes, but since we now have more than just Processes, a separate (non-editable) column is needed to indicate what the underlying Screen is.

You know, I cannot tell that from the screenshot you provided. I think there is a usability problem here that users cannot tell the type of the screen ("Processes") from the screen with the custom name. And the column of available screens won't help with the problem.

My idea is to list the active screens with the type of the screen as well as the name. Under the "Active Screens" heading, show something like this:

Main (Processes)
I/O (Processes)
Cgroups (Cgroups)
Filesystems (Filesystems)

The "Available Screens" should only be there if the user has pressed the "New" button to select a screen they wish to add.

[Explorer09]

P.S. From this screenshot I saw another confusion. When the "Filesystems" screen is highlighted in the "Active" column, the "Available" column does not reflect to what the type of the "Active" screen is. Here, "Processes" is highlighted but I expect "Filesystems" be highlighted. I'm just to tell there is usability bug even with the current approach of listing available screens and available columns separately.

[natoscott]

That's correct - making it a two-step process is the tradeoff to reduce the busy-ness of the Screens setup page.

[BenBE]

[…] is confusing UI design, I don't think we can go that path.

We can, but we absolutely should not … ;-)

Jokes aside.

I've been thinking about some minimalistic notion of "dialogs" for a while, like Midnight Commander does for various operations that require user input. In the case of htop we could do something akin to:

   0[||||||||||||          42%] Load: 0.00 0.00 0.00

   Setup
Categories       +-> Screen setup for Main [Processes] ---------------
Display Options  | Active Columns       Available Columns
Header Layout    | PID                  PID - Process/thread ID
Meters           | PPID                 STATE - Process state
Screens          | USER                 PPID - Parent process ID
Colors           | PRIORITY             PGRP - Process group ID
                 | …                    …

On the main Screens selector you just have the list of screens and the list of screen types available:

   0[||||||||||||          42%] Load: 0.00 0.00 0.00

   Setup
Categories       Screens                Screen Types
Display Options  Main [Process]         Processes
Header Layout    I/O [Process]          CGroups
Meters           Delay [Process]        Network
Screens          Network [Network]      Storage
Colors           Mounts [Storage]       GPU
                 …                      …

That way the UI is minimized for the actual task at hand and the user is only presented with the options necessary at each moment.

[Explorer09]

@BenBE I'm not sure if I get it. The "Screen" and "Screen Types" lists in your UI proposal looks okay, but I have one question:
When is the "Screen Types" list shown, and how would the user's flow be for adding a screen, modifying a screen, and deleting a screen?

@natoscott In your proposal, how would the user's flow be for deleting a screen? How about renaming a screen (changing the display name of the tab)?

[BenBE]

@BenBE I'm not sure if I get it. The "Screen" and "Screen Types" lists in your UI proposal looks okay, but I have one question: When is the "Screen Types" list shown, and how would the user's flow be for adding a screen, modifying a screen, and deleting a screen?

Adding screen: Press function key according to Status Bar, enter name, select type.

Renaming screen: Press function key according to Status Bar, update name, confirm with Enter.

Modifying type: Ensure screen has no columns, select new type from list of available types

Delete screen: Press function key according to Status Bar.

Add/Modify/Remove columns: Navigate to screen, hit Enter.

[Explorer09]

Modifying type: Ensure screen has no columns, select new type from list of available types

My expectation is the screen type would not be modifiable after creation. The user would have to delete the screen and then create another with the right type.
The reason? There would be a lot of confusion if we allow the user to modify the type, when the user would see all the columns they have customized just go away, without a warning or a backtrack.

[BenBE]

Modifying type: Ensure screen has no columns, select new type from list of available types

My expectation is the screen type would not be modifiable after creation. The user would have to delete the screen and then create another with the right type. The reason? There would be a lot of confusion if we allow the user to modify the type, when the user would see all the columns they have customized just go away, without a warning or a backtrack.

Thus my note about "requires all columns deleted" for changing the type. Effectively boils down to deleting and re-creating that screen (sans re-entering the name).

[natoscott]

@BenBE interesting stuff - I like your idea of dialogs.

It's quite a change from the current model though, so I'm not going to attempt it within this PR. :)

@natoscott how would the user's flow be for deleting a screen? How about renaming a screen (changing the display name of the tab)?

No change to how its done today from ScreensPanel. It can also be achieved from the (new) ScreenTabsPanel now, for platforms with dynamic screens.

I've got an initial implementation going, working sufficiently well that the interaction can be experimented with and it feels quite intuitive to me (and is consistent with existing htop UI). There's still some bugs to be sorted out though, I'll continue working on those.

[BenBE]

LGTM.

[natoscott]

This is merged - if there's any platform specific build issues (Linux and PCP only verified so far), please let me know.

[Explorer09]

Ah. I missed the chance to review this code a little bit. There are a few styling issues that I'm not sure I like it. Would you mind if I make pull requests as follows ups of this one?

Here are two issues I can see at the moment:

1. The printBytes, printCount, printPercentage, printTime etc. functions should reside in the RowField class, not on the Row class, as these functions are more for printing fields and not for printing a row.
2. Would you mind if I rename the class RowField to just Field (RowField.h to Field.h)? Because I still believe the name RowField is silly. Also that enum type should be FieldID. I expect that Field refer to a class and not an enum or an identifier. How do you think?

[natoscott]

@Explorer09 personally, I don't think those changes will be helpful - the reason for the RowField naming is to match up with the ProcessField naming, 'Field' is maybe a little too vague IMO.

[Explorer09]

@natoscott I know the ProcessField name. And I think naming the new enum type RowField is no longer intuitive now. I was saying that the enum should be named FieldID so that the name Field could refer to a class or a module on its own.

ProcessField will inherit FieldID (becoming the same integer/enum type). There is no need to change the ProcessField name for now.