Add "Automatically save configuration/view on exit" option #1136
Conversation
| if (saveOk < 0) { | ||
| xSnprintf(msg, 1024, "Error saving to %s: %s", this->settings->filename, strerror(-saveOk)); | ||
| attrset(CRT_colors[FAILED_SEARCH]); | ||
| } else { | ||
| xSnprintf(msg, 1024, "Saved to %s", this->settings->filename); | ||
| attrset(CRT_colors[FUNCTION_BAR]); | ||
| } | ||
|
|
||
| mvhline(LINES - 1, 0, ' ', COLS); | ||
| mvaddstr(LINES - 1, 0, msg); | ||
| attrset(CRT_colors[RESET_COLOR]); |
There was a problem hiding this comment.
@fasterit @natoscott Not sure about this part. Technically correct, but …
- The wording should probably be more clear?
Successfully saved configuration to %svs.Failed to save configuration to %s. - (Mis)using the function bar in this way seems kinda dirty.
- Error messages should be mirrored to
stderr, so they are included in crash reports (just in case).
There was a problem hiding this comment.
This is solving a very, very niche problem. Hence I don't like this PR.
@arp242: Please just chmod -w your htoprc file.
/DLange
There was a problem hiding this comment.
But then I can never save/update it without mucking about with file permissions, and provisioning new machines is more difficult.
I don't know how "niche" it is; I'm pretty sure I added it to me ~/TODO a few years back after I saw some other people complain about it too, I think it was on Lobsters, but I can't find it right now; did find some people complaining about it on HN: https://news.ycombinator.com/item?id=24344542
The automatic saving is pretty unconventional; I don't know of any other (terminal) program that works like this. And when I first used htop I was pretty confused by it ("did it save? How does this work?") Even without a setting, an explicit "Save + feedback" is a good thing to have IMO.
(Mis)using the function bar in this way seems kinda dirty.
Yeah, but I don't know where else it can be printed. The header can be hidden now, so you can't really use that, and this is the only thing available really.
There was a problem hiding this comment.
Please just
chmod -wyourhtoprcfile.
On top ot the above concerns, htop will actually complain if you do this:
Can not save configuration to /home/novenary/.config/htop/htoprc: Permission denied
There was a problem hiding this comment.
Please just
chmod -wyourhtoprcfile.On top ot the above concerns, htop will actually complain if you do this:
Can not save configuration to /home/novenary/.config/htop/htoprc: Permission denied
Yes, of course, because the expected default for nearly two decades now is that your configuration is saved.
|
(Continuing this as a top-level comment because it's not relevant to the code review)
That's fine. And that's also why this PR exists: to add the possibility of opting out of this behavior. If the configuration file changes silently during regular use, that makes it impractical to check it into version control and provision a common configuration to multiple machines with the reasonable expectation that they're going to remain in sync. This is very common practice, especially for TUI utilities like htop. |
I've long since been annoyed by htop's automatic saving of everything I do, and much prefer it to always open it to the default view I configured. This adds a new option to disable this. Because you still want to be able to save the configuration file it also adds F4 to save the file. The error message or some feedback is displayed until the next redraw in the function bar; this was the best place I could figure out to draw this.
|
I have some comments about this PR:
The best place to implement the write permission test is in the static bool Settings_read(Settings* this, const char* fileName, unsigned int initialCpuCount) {
FILE* fd = fopen(fileName, "r+");
if (!fd) {
switch (errno) {
case EISDIR:
case ENAMETOOLONG:
case ENOENT:
return false;
default:
this->readOnly = true;
fd = fopen(fileName, "r");
}
}
if (!fd)
return false;
... |
|
While htop has a "read-only" mode it's still different from what this PR is asking for. Thus just going into that read-only mode when the configuration is write-protected, is not gonna cut it. And as @fasterit already mentioned, this feature is quite the opposite from what the default has been for nearly two decades. The only sensible way to go forward here would be to suppress the error message when trying to save the configuration. Which on the other hand is hiding errors and thus would be bad style. Or to summarize: I don't see any sensible compromise for this PR to not subvert user expectations or suppress expected error messages and be small enough to justify changing htop's behavior for. |
|
I do not believe that anyone asked to change the default behavior. We just want an option to opt out of automatic saving, and making the configuration file read-only is not a reasonable way to do that.
I am very confused by this statement. How is adding an opt-out going to subvert expectations? |
Think about the effect the feature has when it is activated by accident. This will lead to quite some surprised users and potentially unnecessary bug reports. Not to mention: I have exactly this situation with a different program right now where configuration changes are not kept despite all options showing that they should be stored. It's kinda annoying behavior and hard to debug. |
|
I feel like this is a bit of an extreme position. I understand where you're coming from, but it also feels like this is underestimating the user's intelligence. There's a line to be drawn somewhere between making a useful tool, and one that protects its users from themselves. If this is still a concern, I would be happy with not exposing the option in the UI. Don't even give it a CLI flag, but document it in the manpage. Advanced users who want this feature are more likely to read the documentation or find this PR, set the flag in their config, and move on. I still find this a lot better than having to |
|
Would a |
|
I'd be ok with handling a read-only config file more cleverly as laid out in #1136 (comment) by @Explorer09. The |
No worries, and no need to apologize. This PR is likely the only contribution I'll ever make to htop and I'm not the one who will have to deal with things next year, in five years' time, etc. Reading Explorer09's comment, I think they're saying essentially the same: add a flag for it. Their first point ("save in real time") would also be nice, but seems out of scope and something that should be a different patch, I think? Either way, @BenBE didn't seem on-board with that suggestion? If the entire proposal is a non-starter then that's okay too; although I'd obviously prefer some solution – just wondering if there's some way to make it work and keep everyone happy. |
|
The feature in this PR is quite niche and thus any code for it has to potentially be warranted by something else too. Thus as said in my previous comment, the "overall don't save config option" is a non-starter (for the mentioned reasons) and the command line flag, while eliminating the core risks I see with the setting, suffers from the drawbacks mentioned by @fasterit. Overall this is not really taking off in any direction as-is and I agree with @fasterit that a read-only configuration should be handled more gracefully instead. |
|
Adding a command line flag is not much better than making your config file read-only. In order to persist that preference, you'd have to put it in a wrapper script ahead of htop in your PATH, and this never feels like a good solution, especially for a program that has a config file. Then you're back to square one: whenever you actually want to persist configuration changes, you need to undo this somehow; and remember why it's not saving your configuration, which is an unnecessary cognitive load and waste of time. I've already made a proposal in #1136 (comment): if you're worried that users might accidentally turn this on (which is a legitimate concern), make it harder or impossible to do that. You can either do as I suggested previously (don't expose the option in the UI) or show a confirmation prompt when attempting to enable the option.
This patch looks trivial, and I don't think the use-case is niche either. The fact that someone actually took the time to submit a patch for this, and that others such as myself are commenting on this PR, shows that there is demand. |
|
I am closing the PR as we have a team decision to not implement this. |
The reason I voted for the feature is that sometimes the file system just isn't writable. (E.g. on a live system or a remote NFS mount point) So And yes, I expect a wrapper script be used here, but why not?
I'm not sure I get what you mean. My idea was to automatically save the configuration unless the user opts out. In my idea there would be no UI to manually save the config as it is not needed. BenBE
I agree too. |
Live systems typically give you a writable home in tmpfs, so this is not really an issue. In general, I'm not sure why you'd want your home in a read-only filesystem. I have a very specific use-case where htoprc will be in one though: home-manager's htop module. This puts your config file in a RO filesystem by design, but it is still controlled by user configuration. But this isn't really what this PR was originally about. It's for the case where you simply don't want htop to be clever, you already have a configuration you like, and you don't want it to change even if you make random changes at runtime because you occasionally need them. Examples:
In both of these cases, with the autosave behavior, I have to revert those settings to my preference manually. With autosave off, I know that the next time I use htop, my preferences will be restored without further intervention. I've also explained that I like to put my configuration files in a git repo to easily share them between computers. A program that changes its configuration unprompted interferes with this somewhat.
This is for when I want to persist a change. Say I change my mind about my preferred sorting order. With autosave disabled, I need a way to ask htop to actually save. The less friction, the better. Hence a single button to save preferences when autosave is off would be best. It certainly looks like our use-cases are different. I've already explained why I want this in this thread, and offered compromises, but it seems @BenBE and @fasterit are not willing to consider them, so I don't think I will pursue this any further. Unfortunately, this whole situation (combined with reading other issue threads in this repo) leaves me bitter. htop is a tool that I've relied on for over a decade. Seeing that it was handed over to maintainers that are apparently hostile to their users is disappointing to say the least, and there are sadly no real alternatives to turn to. You probably won't be seeing me around here again.
I'm not sure what you mean with "more graceful handling", but I hope you won't introduce an intrusive warning for this. Printing to stderr on exit is sufficient, please don't make the existing workaround any worse than it has to be. |
Okay, fair enough; thanks for considering.
As the author of this patch I mostly agree with you about the patch itself and we have the same reasons for wanting it. But I don't see why anyone should be "bitter" about not including a particular feature, or why it's "hostile". I'm not the one doing the legwork on htop, nor are you. When it comes to open source stuff like this I firmly believe that whomever does the work gets to decide, especially if it's done on a volunteer basis like htop. Who am I to decide how they should maintain htop? I maintain some other projects that see quite a lot of users, and I've needed to reject loads of stuff over the years. Including everything everyone wants would turn it in to a huge mess, especially after several decades. It's a matter of taste to decide what to include and what not to include; clearly my sense of taste differs on this specific subject, and that's okay. Rejecting patches is one of my least favourite responsibilities because I appreciate that people have put in their own spare time, but it really does need to be done. Am I "disappointed" the patch is rejected? I guess – obviously I would have preferred it would be included in some form. But why should I be "bitter" at the people who spent countless hours of their spare time maintaining a useful piece of software I frequently use? How is it "hostile" to maintain such software in a way you don't like? If anything I feel the exact opposite: I'm happy and thankful it's being maintained at all, and I'm happy and thankful that my patch was given serious consideration. I don't see why BenBE, fasterit, or anyone else owes me anything more than that. At the end of the day they're correct is a fairly minor thing. A minor thing that bugs me, but still a minor thing. If you're dissatisfied by how it's maintained then you're free to take the source and maintain "htop-ng" or "neohtop". From what I've seen >95% of the "zomg the maintainers of that project are assholes, here's a better fork!" are dead within the year because turns out that maintaining a project is a lot of hard and often boring work. I'm not just talking about the "writing code" part, also about the "people demanding you include a feature and call you a jerk if you don't"-part. Maintaining a project with tens of thousands – if not hundreds of thousands or even millions – of users is different than putting something you made on GitHub that few people look at, even more so if it's a complex program that people interact with directly (like htop). It's the difference between a WhatsApp group with 10 of your friends and running Twitter: basically the same thing (people exchanging messages), except it's not the same thing at all. |
|
@9ary
Okay. I get it now. A simple method to manually save the configuration, similar to how Ctrl-S saves the file in most GUI editor apps, right? Unfortunately this brings more issues to address in terms of user experience:
The third issue I can think of, is not related to UX, but that htop cannot have two sets of codes, one for autosave behavior, and the other for manual save behavior. For the code to be maintainable long term, there can be only one behavior kept, and the other dropped. And it would not just htop would have to make the decision on the behavior. I think many other GUI apps would have to decide between auto save and manual save, too. |
|
I wonder if you guys like this feature if htop is to stick to auto save on exit:
I remember many other apps that have the auto save behavior would have such a Revert button to discard all the changes, and this would work close to the use case you want. |
|
I'm sorry, I didn't think my comment would come out as antagonizing or inflammatory. I was only expressing my disappointment. Let me explain myself a little more: I don't believe in democratic governance of open source projects to a certain extent: the democracy of FOSS is the fork button, and it seems we agree. That said, I believe htop qualifies as "critical infrastructure" (or gets pretty close) given its popularity, so even though I don't think maintainers owe me anything, I still expect them to properly consider to the use-cases of their broad userbase. This feature may seem niche to the maintainers, and while I've done my best to show why I don't think that's the case, I also don't think that would necessarily justify rejecting it if the implementation/maintenance cost is reasonable. The patch itself is trivial (doesn't add meaningful maintenance overhead), and it implements a reasonable feature. htop is somewhat unusual, being a TUI tool that writes its own configuration without asking the user to save, when the majority of others either expect you to edit your own config by hand, or have the ability to change the configuration at runtime and make saving changes a manual step. I find it hard to agree with the maintainers' stance, and they have made it clear that they will not accept a compromise. The proposed workaround is also inappropriate in my opinion: this is the kind of hack I would be using to make proprietary software behave, but not something I should have to do with a FOSS program. That said, it makes for worse UX but doesn't hinder the overall usability of the tool, so I can live with it, as long as #1136 (comment) doesn't imply that the intention is to break it. I think I've already made my case, and given the lack of response to my proposals, it's unlikely that anyone is going to change their mind, so there is nothing left to add really. Apologies if I've seemed rude or aggressive to anyone. Thank you for maintaining htop so I don't have to. ❤️ @Explorer09, what you're suggesting is essentially a "reload config" button, but it's really not what I'm after. I just don't want the configuration file to change unless I take explicit action in that direction. Thanks for trying to find a compromise though. |
|
@9ary And the "compromise" I provided is not "reload config". It's "revert every config change to the state when htop just starts". As the config file could be updated any time because of auto save, a "reload" action would not be sufficient for the use case you needed. Htop would need to keep a copy of the "original" config in memory. |
|
But htop is not a GUI application. I can't reasonably compare it to Firefox, some closer "relatives" would be things like vim (which uses a hand-written configuration file), or weechat (has a config menu, saving config changes is manual by running the I'm fully aware that there are UX benefits to both approaches. I personally like to manage my configuration in a way that's stable, consistent, documentable and reproducible. If I could coerce GUI tools to fit into that model, I would also do that. Note that Firefox, for example, has a mechanism to override preferences on startup (user.js). Any preference that you set in there will override those in prefs.js any time you restart the browser, and Firefox will never make changes to that file on its own.
What you propose is still just a way to revert my changes before I close htop. If I forget to do it then the configuration is written. I don't want that. This isn't a compromise, it's a different feature. |
And there is no way to satisfy both sides. I'm sorry, but the workflow you have doesn't fit the auto save behavior that htop currently has. Changing the behavior of htop would break the expectations of more users than the benefits you would bring. The |
|
@9ary Treating configuration like editing a text file would be a dated approach. And that approach would make sense for less and less applications as most of them are now coded to reflect the configuration changes in real time, and auto-save them. I didn't even mention that if there would be a "Save" button for the configuration, why not a "Save As" button as well. And incorporating these into htop would turn into a feature creep that solves only niche use cases and most users won't care. |
|
@Explorer09 you are either trolling me very hard or completely missing the point of this PR. Text configuration files have been the gold standard for a very long time, and many new applications are still being written to this day using them, because they have many advantages over non-human-editable configuration files. Textual programming languages are not being replaced with flowcharts or whatever for the exact same reason. This conversation is no longer productive, if it ever was, and I am finding it increasingly hard to keep my cool while responding, so I will now unsubscribe. |
And you might not have realized that htop's configuration file was initially not intended to be edited by humans. That's the reason for the first two lines of the file:
I'm just telling you htop's stance and why the PR was rejected. No offense, really. The two workflows (auto-save and the manual save button) are not compatible, and htop can only decide one workflow or the other and we can't satisfy both sides. Many apps that did the auto save behavior didn't even have the option to opt out (it makes no sense to have it). |
I've long since been annoyed by htop's automatic saving of everything I do, and much prefer it to always open it to the default view I configured.
This adds a new option to disable this. Because you still want to be able to save the configuration file it also adds F4 to save the file. The error message or some feedback is displayed until the next redraw in the function bar; this was the best place I could figure out to draw this.