-
Notifications
You must be signed in to change notification settings - Fork 973
"Clear browsing data" user settings #4082
Comments
@bsclifton I would like to take this up. 😄 I did some initial investigation. Seems like the state of the setting is not being persisted. Could you point me to a example where the state is being persisted and also retrieved? Will persisting the settings at https://github.com/brave/browser-laptop/blob/master/js/components/clearBrowsingDataPanel.js#L32 be sufficient? |
Hi @maaz93! Let's do this 😄 So our persistent session contains Window settings and Application settings. This requested feature is definitely an Application setting, since it would be global. Here's our documented storage format (you're interested in the AppStore): So what we could do would be to add a new item there called
|
Of course, Here, you have an opportunity (since
Notice that we try to retrieve the values from the AppState (the data persisted by the AppStore). The first time you run this, it'll be undefined since the data doesn't exist. If you wanted to provide default values, you absolutely could inside of the clearBrowsingDataPanel control itself by adding a handler for React's
And then (fun part)- if the value changes at all (you can check in the method you originally linked to, you'd want to persist this new value by creating a new action (
|
@maaz93 I know I captured a lot of information (and you may already know large chunks of this), but I hope these posts are helpful. Please ask me any questions- I'd love to help more 😄 (Permission-wise, I can't assign you to the issue, so I assigned the issue to myself and I'll work with you) |
@bsclifton That's so clear! 😄 I think this is sufficient to start off. I had one question though. There are 3 options in the History tab, that open up the same dialog with different options selected by default. If I were to implement this default state persistence, would I have to maintain a state for each of these options or I would do something on the lines of a logical OR between the defaultPersistedState and the expected dialogState? For eg. If I choose |
@maaz93 good find! This does introduce an edge case If the user picks "Clear Cache" from the menu, it disables everything except clear cache. We must leave it like this or else folks may accidentally erase other settings. In these cases (where a modal is initialized with everything disabled except for X), the state of each setting should NOT persist itself (when the modal is closed, etc) This would be a great one to write a test case for 😄 I can help with that too, when you get further along (let me know!) |
@bsclifton I'm a little confused with this statement
There are 3 options in the browser History menu, Clear History, Clear Cache and Clear All Cookies and Site Data. And clicking on these results in the following pre-populated dialogs, respectively Clear History In all these scenarios, some options are checked by default. In what scenario would I persist the options then? |
@maaz93 interesting... so we'll have to think this through Besides picking from the menu, you can also load this screen through preferences: From here, it would be great to persist the settings. The menus though are almost describing what they should do. "Clear Cache" should never default to also having "Saved site settings + permissions" checked for example. Perhaps the items from the menu would be better served by having a yes/no confirm box, rather than exposing the sliders? The version from the preferences I would feel comfortable recommending the persisting of the data. Before we do anything, I'm going to have to defer to @bradleyrichter. Brad, what do you think about this issue? |
Sorry there is some confusion here. First, we need to reset the menu options as described here: #3093 Then, with that single menu - I think it should be reset after each use because we have the automatic clearing options in the prefs security panel. Does this make sense? |
@maaz93 - you can start with #3093 and then we can figure out how to handle this one @bradleyrichter with regards to this ticket, the feature request was to persist the settings that were chosen. You're proposing that the items which are enabled/disabled in that modal get reset with each use? If so, we can close this ticket as a won't fix I can see value in saving the last selected value, in case the person is clearing something often |
@bsclifton Cool. I'll start off with #3093 Also, just my suggestion on this thread, I'm with @bsclifton and @Kanichiro on the fact that it seems like a useful feature to save the last selected values for the user, as I feel this menu is more accessible than the security panel. 😄 |
Ok. We can save the last settings since it is non-destructive until hitting the go button.
|
@bsclifton I'll be working on this now as #3093 is closed. Will need help writing tests, once I'm done! 😃 |
@maaz93 how's it going? Let me know if you need help 😄 |
@bsclifton I have a working piece of code right now. But, I feel it can be simplified before submitting a PR. I feel I could benefit from another pair of eyes. Could you take a look at it if I pushed a commit to my forked repo? 😄 |
@maaz93 sure thing 😄 let me know your branch name and I'll check it out |
@bsclifton Here it is. The major issue that I'm facing is having two props, namely clearBrowsingDataDetail and defaultValues, that need to be selectively bound to the SwitchControl's checkedOn prop. Let me know your thoughts! 😄 |
@maaz93 I left you some comments on your commit maazadeeb@97537e3 Please read through them and let me know if it makes sense. Basically, we should remove the existing code which is saving what was chosen to the windowState (we don't want this, since your changes persist the data to the APP state, which is shared between windows). That will resolve the problem you've found 😄 |
@bsclifton I went through the comments. I didn't think that I could delete the existing code in favor of my change. That makes so much more sense! We delete the code to store in the windowStore and persist the settings on clicking Clear directly to appStore. Thanks! |
@bsclifton I've been sitting on this bug a long time now. 🕐 When I started making some changes, I realised that I didn't get the flow of code. I spent some time reading/experimenting with React. I also understood the Flux architecture that you guys are also following. (Component -> Action -> Dispatcher -> Store -> Component). With this understanding, I have an important question to ask. Is it absolutely necessary for every component in Brave to extend If no, then I think I can cook up a pretty simple solution by not extending it and using this.state, else I'll have to just build on top the existing code and not replace it 😄 |
@maaz93 ImmutableComponent is just an optimization we introduced for components which don't need to track state. If you need to make use of state, please just inherit from React.Conponent 😄 |
@bsclifton I'm finally done with my change! It's at this branch in my fork. Please have a look if you can 😄 . I need some help writing tests now, as mentioned before. 😄 I would like to know what sort of tests I've to write and also how to fix broken tests. |
@maaz93 sorry it's taken me so long to respond; the easiest way I can recommend would be to find your session folder (or the session file) and temporarily move it out of the way. You're on Fedora, right? I believe the session is stored here: Make sure all instances of Brave are closed and then try moving that file (rename as |
@bsclifton No problem. I'm on a Mac. Though the folder structure is not the same, the file name session-store-1 is the same. So I was able to find it. Thanks! 😄 |
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes brave#4082
@Kanichiro this new version hasn't been merged to master yet and definitely hasn't been released yet... are you running from source using the branch we have, You can always download any version you'd like from the releases page: Apologies that you've been having a bad time; I promise that there is a fix coming |
@Kanichiro regarding the restart, that was introduced by @diracdeltas a while back (September 2016) with 9f7073c It's done as a work-around to clear WebRTC device IDs. I'll create an issue sharing that you feel it's inappropriate to prompt for restart. In the meantime, just choose NO and you'll get the same behavior as before edit: |
@bsclifton Thanks, I've downloaded and installed my former version of Brave (0.12.0 for Linux). Much, much appreciated for this link to get this older version! I think I'll keep stick with this version until you give the OK. ps: I kept the backup copy just in case! |
Fixed with #6026 which was merged into branch Test plan
|
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes brave#4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Majorly, the change affects 2 places in the UI. The Clear Browsing Data… option in the History menu and the Clear Browsing Data Now… button in the Security tab in Preferences. We store the state of the toggled options when the user clears anytime, so that the next time he/she opens this dialog, the options are pre-populated as per the last selected options From a technical perspective, we have moved away from saving the current state of the SwitchControls in the windowState. We only save the visibility on the windowState. The whole state is managed by the panel itself and just updates the appState on clicking clear. Fixes #4082
Did you search for similar issues before submitting this one?
Yes
Describe the issue you encountered:
Expected behavior:
Settings to stay On
Linux Mint 18 - Cinnamon
See screenshot
see screen shots
Everytime I select "Clear browsing data," I am forced to move the selector bars for Browser History & Download History from off to on. I tend to delete these items often, even hourly at times as I can visit a lot of news sites and I prefer to delete what these sites store on my computer.
Please allow your Users to make these settings default or not. I should be able to just select ON as my prefer setting and have them stay ON!
The text was updated successfully, but these errors were encountered: