Adds about:history, sets back/forward nav limit, more efficient dynamic menus #3206
Conversation
| // Only update menu when necessary | ||
|
|
||
| if (updated.settings || updated.closedFrames) { | ||
| console.log('rebuilding history menu') |
There was a problem hiding this comment.
logs from debugging? Do we need/want these? We really need to add our own log method with more control over output granularity
|
mentioned via DM, seems to be a bug with clean session when there are no sites. After navigating to a single site it works for all future restarts. |
| @@ -110,6 +110,9 @@ const messages = { | |||
| RESPONSE_WINDOW_STATE: _, | |||
| LAST_WINDOW_STATE: _, | |||
| UNDO_CLOSED_WINDOW: _, | |||
| // Menu rebuilding | |||
| REQUEST_WINDOW_STATE_FOR_MENU: _, | |||
| RESPONSE_WINDOW_STATE_FOR_MENU: _, | |||
There was a problem hiding this comment.
If you're only using closedFrames from windowState then I think you should call this:
REQUEST_WINDOW_CLOSED_FRAMES
RESPONSE_WINDOW_CLOSED_FRAMES
And then update accordingly.
There was a problem hiding this comment.
actually call it something like request menu data for window, and only include closed frames in it.
There was a problem hiding this comment.
Just noticed we also have UPDATE_APP_MENU you should rename that to be more specific to be about bookmarks, and then maybe that is an indication that you should be more specific for perf reasons in the message names as I originally suggested in the first message in this thread.
|
Looking great, I can't wait to get this in! |
|
OK most of the feedback implemented and ready for a re-review 😄 Each follow-up commit should have the relevant notes Next up- focusing on the concerns with the History page (breaking code into a control, implementing context menu, etc) |
| RESPONSE_WINDOW_STATE_FOR_MENU: _, | ||
| REQUEST_MENU_DATA_FOR_WINDOW: _, | ||
| RESPONSE_MENU_DATA_FOR_WINDOW: _, | ||
| UPDATE_MENU_BOOKMARKED_STATUS: _, /** @arg {Object} currently only has a boolean "bookmarked" */ |
There was a problem hiding this comment.
nit documentation bug, I think this arg is actually a boolean type and not an Object
…double click entries to launch), - History menu now has a "Show Full History" which pulls up about:history. Fixes #444 - History in back/forward nav buttons is limited to 10. Fixes #2889 - History menu shows recently closed frames (based on WindowState) - Broke methods out from menu.js to a helper file (menuUtil.js) - menuUtil tests now use mockery (and work properly), stubbing electron (we can also unit test other electron dependent code like this). - history and bookmarks menus are now dynamic and uses clear/insert calls instead of rebuilding all menu items. Fixes #1993 - history menu now considers modifier keys (similar to bookmarks menu)
- Renamed IPC message names to be more specific
- Only closedFrames is sent to menu update now (and it's immutable, matching the other param)
- Added .get('location') to the bookmarks/history items for a fallback title
- "Show History" is now always present on back/forward nav button long press menu (thanks for reviewing, @bradleyrichter)
- Fixed bug where only 9 items would show on back/forward nav button long press
- Noticed something I missed; when closedFrames changes, it needs to trigger a menu update. Put placeholder TODO.
- Menu now rebuilds on BrowserWindow focus - Fixed bug where "Bookmark Page" menu item's check status could be out of sync (got updated via AppStore)
- Fixed issue where empty session would not initialize menu - Closing frames will now fire Menu update (for the History > Recently Closed)
| windowActions.setBookmarkDetail(siteDetail, siteDetail) | ||
| // Update checked/unchecked status in the Bookmarks menu | ||
| ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) |
There was a problem hiding this comment.
similar to the issue with DATA_FOR_WINDOW, we're using ipc messages in a lot of places where we could (and should I think) just be using actions. The windowActions are already sent to the appStore via the dispatcher, we're just not handling them there. Ultimately this seems a little backwards to me anyway because bookmarks are part of appState, not windowState and the bookmarked state of the current url should come from there. I realize there are legacy reasons for all of this, but it would really be nice to clean some of it up or at the very least not add to it (in the DATA_FOR_WINDOW case)
|
@bsclifton Great! Thanks! This is one of my (ever-dwindling) checklist items of "must be implemented before Brave becomes my default browser". :-D Looking forward to it! 👍 |
|
it does not, but I was going to add in the necessary methods in electron to make that functionality possible so we can add it in the next few releases |
git rebase -ito squash commits if needed.@bridiver I could definitely use your help profiling the menu calls 😄
For the dynamic menu, I ended up not going with removeItem...I had made electron changes, but when using the new method, electron does NOT actually remove the item from the menu object... it only hides it from the menu. Additionally, I was seeing memory leaks when I profiled this, even with electron.Menu.destroy (MenuItem does not have a destroy)
I'd also like to cc @bbondy @darkdh @diracdeltas for feedback on the changes and @alexwykoff as a heads up (I can create QA steps which outline the changes)