Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Adds about:history, sets back/forward nav limit, more efficient dynamic menus #3206

Merged
merged 6 commits into from
Aug 17, 2016
Merged

Adds about:history, sets back/forward nav limit, more efficient dynamic menus #3206

merged 6 commits into from
Aug 17, 2016

Conversation

bsclifton
Copy link
Member

@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)

// Only update menu when necessary

if (updated.settings || updated.closedFrames) {
console.log('rebuilding history menu')
Copy link
Collaborator

Choose a reason for hiding this comment

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

logs from debugging? Do we need/want these? We really need to add our own log method with more control over output granularity

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 😄 Removed!

@bbondy
Copy link
Member

bbondy commented Aug 16, 2016

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: _,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

actually call it something like request menu data for window, and only include closed frames in it.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@bbondy
Copy link
Member

bbondy commented Aug 16, 2016

Looking great, I can't wait to get this in!
I made a bunch of small asks but I think we can get this in soon. Probably after the next commit.

@bsclifton
Copy link
Member Author

bsclifton commented Aug 17, 2016

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" */
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

@bridiver bridiver Aug 17, 2016

Choose a reason for hiding this comment

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

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
Copy link
Member Author

bsclifton commented Aug 18, 2016

@echosa great catch- no, it does not. It seems that #444 was used to not only track an about:history page but also restoring the history for a reopened tab.

I went ahead and re-opened your original request #2702 and can take a look at that while I'm in the history code 😄

@echosa
Copy link
Contributor

echosa commented Aug 18, 2016

@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! 👍

@bridiver
Copy link
Collaborator

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

@bsclifton bsclifton mentioned this pull request Aug 18, 2016
@bsclifton bsclifton deleted the back-forward-nav-enhancements branch August 26, 2016 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.