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.

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

Jump to bottom

RowField class / module improvements #1295

Closed
wants to merge 1 commit into from
Closed

RowField class / module improvements #1295

wants to merge 1 commit into from

Conversation

Explorer09
Copy link
Contributor

No description provided.

@Explorer09
Merge 'RowField' and 'ProcessField' types to 'FieldID'
6d091a8 
Both types are integer identifiers for fields. As the dynamic columns
and process fields share the same identifier space, it makes no sense to
split the types to two.

This commit also frees up the 'Field' name so it can refer to an object
or class rather than an ID in the future.

Signed-off-by: Kang-Che Sung <[email protected]>
natoscott
natoscott reviewed Aug 31, 2023
View reviewed changes
RowField.h
@@ -51,6 +51,6 @@ typedef enum ReservedFields_ {

/* Follow ReservedField entries with dynamic fields defined at runtime */
#define ROW_DYNAMIC_FIELDS LAST_RESERVED_FIELD
typedef int32_t RowField;
typedef int32_t FieldID;

Copy link
Member

Choose a reason for hiding this comment

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

If we were going to do this, the filename here would have to change.

But (as I said before you wrote this patch) - IMO "Field" is too generic a term. Conceptually, there are individual 'fields' within Meters for example.

So I don't think we should go this route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. And I plan the next commit would be to rename RowField to Field for the entire module.
Speaking of meters, the current code base refer to what you called 'fields' as 'items', so there is no confusion.
If you would complain that name being too generic, I would argue the same for the Table and Row class names.

Copy link
Member

Choose a reason for hiding this comment

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

There is confusion here, for the reasons I've pointed out.

If we ask anyone what part of the htop UI relates to 'tables' and 'rows', they'll point out the exact right thing every time - trying to compare that to 'Fields' just doesn't work, sorry.

Its a NAK from me on this proposed naming change.

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 the 'Field' would refer to the table cell. And it's not weird to call it a 'Field' anyway.

https://en.wikipedia.org/wiki/Column_(database)

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I understand where you're coming from - but it is quite ambiguous even within htop. Definitely not "wierd" though - 'too generic' was the phrase used.

@Explorer09
Copy link
Contributor Author

  1. I still don't like the 'Field' name to refer to the enum or identifier. And I think the type merge (RowField and ProcessField) is still necessary for ease of future maintanance.
  2. Do you have a better name to suggest than RowField or Field? I was considering the Column name in place of Field. This will match the naming of Table and Row and might be easier to grasp.
  3. This PR is still a draft, and I'll gather some comments before bringing in new commits. That are some more code mess that I will clean it up.

@Explorer09
Copy link
Contributor Author

My current plan:

  • Rename RowField module to something like Field or Column that would make it more intuitive.
  • Move the following to the 'Field' module: Row_printBytes, Row_printKBytes, Row_printCount, Row_printTime, Row_printRate, Row_printPercentage
  • RowField_keyAt, RowField_alignedTitle

@BenBE
Copy link
Member

BenBE commented Sep 2, 2023

My current plan:

  • Rename RowField module to something like Field or Column that would make it more intuitive.

Will likely get the PR rejected instantly. Both @natoscott and I object to this change.

  • Move the following to the 'Field' module: Row_printBytes, Row_printKBytes, Row_printCount, Row_printTime, Row_printRate, Row_printPercentage
  • RowField_keyAt, RowField_alignedTitle

Make that both RowField and we may get to talk about it.

@Explorer09
Copy link
Contributor Author

@BenBE How about the name RowFieldID in the RowField module?

@Explorer09
Copy link
Contributor Author

Closing this one temporarily as I don't know how to improve the naming with the current state of main...

@Explorer09 Explorer09 closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natoscott natoscott natoscott left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Explorer09 @BenBE @natoscott