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.

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

Feat: Initial multi-select support for events #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mitchcapper
Copy link
Contributor

Pin/Delete/Export/Create Rules all work as expected
Closes httptoolkit/httptoolkit#76

This adds a draft/sample for multi-select for rows, not sure if this is the direction one wants to go so certainly is not polished.

It implements:

  • standard windows explorer control/shift behavior for selecting
  • Exporting selected rows to HAR
  • Deleting selected rows
  • (un)Pinning selected rows
  • Creating mock rules for selected rows
  • Only enabled when the checkbox column is checked
  • Context menu options should work as well due to how they are bound.
  • makes onPin behave like onDelete for central behavior purposes

Things that should be done:

  • Keep track of what rows are selected, this would prevent some of the constant enumerating, and more importantly give a count number of how many are selected that could be used other places (context menus, etc rather than saying 'Delete This Rule, Delete 5 rules', show on the right view pane also a count of how many are selected when > 1

  • Updating titles/context menu text to reflect multiple events selected

  • There is an actual checkbox mode can be enabled with setting USE_MULTI_SELECT_CHECKBOXES=true in view-event-list.ts. This enables standard checkboxes next to each row. This might be useful for people in the same way file explorer can have checkboxes enabled, also for mobile (is there mobile ?)

  • The multiSelected rows that are not the actual selection have the background/foreground set to the same as the selected row just without the bold. By default though this difference is hard to notice unless on high contrast, something should be changed for the CSS for this.

  • OnDelete might be slightly hokey given it runs all the delete logic for selecting the next event after deleting each event but seems to work (mass scale though might hit perf issues).

  • The export to HAR is hokey, by default the buttons for actions are the right side buttons, but there is no singular export to har button so I used the main button rather than add a new one. Downside is if you enable multi-select (check the box in the column header) then click on just one random event and go to export it exports one not all. Likely a har button added to the right side for when mutli-events are selected would be good.

  • It uses the filtered events for nearly all queries/actions. This may not make sense, especially where things that are filtered, then multiSelected, then not shown in the current filter stay selected. They should likely have the selection cleared. For what events most actions effect GetMultiselectSelectedBulkEvents handles the initial filtering to make it easier to adjust that pattern. view-event-list-buttons should use GetMultiselectSelectedBulkEvents but doesn't as I wasn't sure the best way to surface it other than passing the function all the way down the calls.

  • Right clicking to use a context menu when multi-select is enabled on a non multi-selected row can produce interesting results as it generally effects all selected items + row you are calling the menu on.

  • Technically you can multi-select some rows, control click a row then control + click it again to deselect it and the right pane goes away so you can't do most actions through that, even though there are still rows selected.

  • The checkbox alignment is the header uses some negative margin alignment. This was mainly done for when USE_MULTI_SELECT_CHECKBOXES is enabled it only takes up more space on each row when the checkbox in the header is clicked.

  • Keyboard multi-select was not implemented but is fairly straightforward.

  • Standard I don't know react so things certainly may not be done optimally. My few methods with actual in the name are poor form, but allows for minimal changes so should be refactored. The RowCheckbox in view-event-list in particular is a bit odd: first to be able to use a more contextually correct name of onChecked than onChanged for the event, I had to name it whenChanged otherwise react whines. Secondly I had to double specify the type for the props which I don't think I should have to do but couldn't figure out another way without creating the interface and using it.

@mitchcapper
Copy link
Contributor Author

crap of course I selected create PR and not create draft, not sure anyone would be confused and merge this as is in though so :)

@mitchcapper mitchcapper changed the title Initial multi-select support for events Feat: Initial multi-select support for events Oct 4, 2023
Pin/Delete/Export/Create Rules all work as expected
@pimterry
Copy link
Member

pimterry commented Oct 9, 2023

Nice stuff @mitchcapper! This is definitely a change I'm interested in and there's been lots of demand for this for a while, thanks for looking into it.

I think there's a few things here that will need work to make this mergable though:

  • I don't think checkboxes or actually any selection elements should be shown at all (including in the header). This should really behave like a native list so far as possible, using multi-selection via ctrl/cmd+click and shift.
  • We'll probably want a selection indication for the selected rows without checkboxes - I think we should be using the same selection styles (bold + lighter/darker background) we currently use for the single selection, applied to all rows. That's selection styles, not focus styles, which shouldn't change (you can multi-select, you can't multi-focus)
  • We should keep the structure of our state consistent, in one format, with just one way to select things (1+ things). There's a few places here where it seems like the state has become "either one selected item, or MULTI_SELECT=true and a list of items that come from somewhere else". Instead, we avoid modal logic (no multiselect boolean parameters, or anything similar!) and we should just always pass a list of elements (usually 1, but 2+ during multiselect). That will result in one set of consistent logic in each method that works the same for all cases. OnDelete here for example uses either an event argument or a GetMultiselectSelectedBulkEvents list of events when multi-select is active. Instead it should just always take a list of events and delete them.
  • The selection state should live on the view page, not on the events themselves. It's not part of the traffic data model, it's part of the current view page state, and we should be able to track it there directly. Effectively selectedEvent should be selectedEvents, and should always return a list of 0+ events, using some state stored in that component.
  • All that said, there's one awkward bit: the URL currently holds the selected event, which won't scale for many selected events. We will likely need some separate special URL path to handle that (/view/multi) for example.
  • When multiple elements are selected, we shouldn't show just one on the right any more: we should show a new right pane, that represents multiple selected elements (e.g. "Two events selected" text above a set of big buttons for actions). My email client allows multi-selection, and does exactly that:
    Screenshot from 2023-10-09 19-13-42

Does that all make sense?

@mitchcapper
Copy link
Contributor Author

Agree with your points. Feedback on a few things:

  • the global MULTI_SELECT bool was more showing a way the entire feature could be turned on and off but similar can just be done in the select handler if you decide to give users a choice.
  • I think selected and focused should be distinct as you mention. I do believe though it is still valuable to show the focused item. If I am trying to export several queries I may need to see a request to know if I want to add it. I think adding an indicator / count additional ui to indicate the selected info is good. Another example if I want to select many events and delete them all I need to either memorize all of them ahead of time or if focus does change to a specific request I can click into it to make sure it is or is not one I want to delete.
  • if you do go the route of focus / select being distinct I assume we modify event data to transmit both rather than just saying the last item on the list (or what have you) is the focused item.
  • this also solved the url question leaving it to point to the focused item

@pimterry
Copy link
Member

the global MULTI_SELECT bool was more showing a way the entire feature could be turned on and off but similar can just be done in the select handler if you decide to give users a choice.

Nope - if we do this right then I don't think any choice is required (if you don't want it, don't just select multiple requests and nothing will change) and I'm generally keen to avoid too much configuration possibilities, to keep both UX and the code simpler. Any users who really want to do funky things have full freedom to mess with the code themselves.

I think selected and focused should be distinct as you mention.

I think we're using slightly different definitions. The focus state I'm talking about is purely the raw web focus state, i.e. the focus that changes if you press tab, so that either a row is focused, or one of the buttons elsewhere on the page, or an input field, etc. It's totally independent of selection, and doesn't directly control the right pane. For rows, focus is currently shown as little red dotted outline. That moves if you click elsewhere (e.g. expand/collapse a section on the right) and moves with tab/shift-tab. When you click to multi-select, the last row clicked will end up focused, in that sense, but I wasn't suggesting that it would be shown on the right at all.

if you do go the route of focus / select being distinct I assume we modify event data to transmit both rather than just saying the last item on the list (or what have you) is the focused item.
this also solved the url question leaving it to point to the focused item

Given the above, this isn't quite right. I'm not sure what 'event data' is, but I think the only state we need is:

  • A URL parameter (/view/$X) that's either blank (nothing selected), a single event ID, or multi.
    • We use this to jump directly to events, e.g. for breakpoints (which work by navigating to /view/<breakpointed event id>)
    • We also use this to support back/forward nicely, so you can go through your selection history. I think that's probably impractical to do for all multi-selection state, but we should keep it working for normal selection at least.
    • I think via browser history APIs, you might be able to use state to track multi-selection in browser history too, but it's quite tricky to do right, and we should leave this for later.
  • A list of selected items, stored as an observable field in the ViewPage
    • There's maybe an argument for moving this into the UiStore... That would be a larger change but could be interesting as a future extension. In either case though: selection state should be stored as a single list of 0+ events, in one single place within the UI state, nowhere else.

If I am trying to export several queries I may need to see a request to know if I want to add it.

Regardless of the 'focus' terminology, this is a good suggestion (ditto for the equivalent deletion point).

I think we can handle this within a multi-select right pane view though: for example, we could show "5 events selected" and then quick summaries of 5 rows there, which you could expand on mouseover with a little more detail. There are interesting UX options here we can certainly play with.

So yes, that's a feature that totally makes sense to me, but I don't think it's strictly required for a usable first version, and most other software I've used that allows multi-selection doesn't have a solution to this. I think we should plan to improve later and keep this first iteration simple: if you have more than one item selected, the right pane always shows "N items selected" and buttons for bulk actions, nothing else.

We can explore expansions and more complex behaviour in future, once we see get something working, see how it feels in practice, and get real feedback.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pimterry pimterry force-pushed the main branch 2 times, most recently from e0e350c to 1d713c8 Compare May 7, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi selection for intercepted request
3 participants