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

ui: improve active order interactions #2171

Merged
merged 4 commits into from Sep 2, 2023

Conversation

buck54321
Copy link
Member

Allow sweeping of fully-executed orders from active orders list on markets view. Shows a broom icon in the button menu to sweep a single order, and near the header to sweep all retired orders.
image

Show (a copy of) the button menu on hover, allowing the user to perform actions without expanding the active order entry.
image

@@ -377,6 +376,8 @@ tmux new-window -t $SESSION:8 -n "miner" $SHELL
tmux send-keys -t $SESSION:8 "cd ${NODES_ROOT}/harness-ctl" C-m
tmux send-keys -t $SESSION:8 "watch -n 15 ./mine-alpha 1" C-m

tmux select-window -t $SESSION:0
Copy link
Member Author

Choose a reason for hiding this comment

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

@JoeGruffins
Copy link
Member

I noticed the orders come back rather easily, and the sweeper button for all the orders disappears. They all come back if you place an order, or go to a different market and come back.

orderscomebackagain.mp4

Copy link
Contributor

@norwnd norwnd left a comment

Choose a reason for hiding this comment

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

While broom icon looks neat, I think that extra click will start bugging users after they used it a couple of times, which suggests to me some automatic cleanup would be desirable (and I don't see a use-case to remove a particular order from that list but not others, won't /orders page be more appropriate place for filtering orders as desired ?) ... why not clean up all the orders retired more than ~10 mins ago ? Or maybe ~1 min would be enough, I don't see many reasons to keep displaying retired orders at all TBH, except to give the user "a sense of progress/engagement" on a second thought, it is possibly the easiest place to access/examine your recent orders, maybe leaving it there for something on the order of several hours (maybe even 24h) would be more appropriate way to go about it ... but probably better to keep this list reasonably short; any reason not to limit it to, say, all active + 10 latest retired orders (optionally cutting off those older than 24h, to keep it reasonably recent) ?

also (depending on whether you decide to follow through on this ^ suggestion), consider renaming Your Orders section to clarify what exactly it contains, e.g. into Your Recent Orders or maybe Your Outstanding Orders.

Besides, based on my testing I get a sense this feature might be tricky to make working right in all cases:

  • I also see the issue mentioned above
  • popup window, while useful (because it saves that extra click, appreciate that!), feels unnatural to me ... maybe because its size isn't exactly same as that of the row behind it; looking at it 2nd 3rd times no longer feels as bad, but first impression is probably better indicator when testing UI/UX
  • having two consecutive pop-ups is probably not desirable/expected behavior, and feels like a bug (it can hang like this for seconds until I click on something, not sure how to reproduce exactly)

image

  • it looks like Your Orders list is eventually consistent (meaning, it updates eventually, but might reflect old status for a while - on screenshot below it took ~2 minutes to update settling->executed); it's not an issue on its own, but showing the broom in this case might be a bit confusing for the user

image

@chappjc
Copy link
Member

chappjc commented Feb 27, 2023

They all come back if you place an order, or go to a different market and come back.

Yeah, this limits the usefulness of the manual sweep button a lot. I noticed this in diagnosing #2173 because making a new order triggers a table rebuild it seems.

why not clean up all the orders retired more than ~10 mins ago ? Or maybe ~1 min would be enough, I don't see many reasons to keep displaying retired orders at all TBH, except to give the user "a sense of progress/engagement" on a second thought, it is possibly the easiest place to access/examine your recent orders, maybe leaving it there for something on the order of several hours (maybe even 24h) would be more appropriate way to go about it ... but probably better to keep this list reasonably short; any reason not to limit it to, say, all active + 10 latest retired orders (optionally cutting off those older than 24h, to keep it reasonably recent) ?

"Retired" (removed from the trades map on the backend, not just fully executed and settled) orders will never show on the markets page, at least after you refresh the page, because these are no longer tracked order. Such retired orders can only be found on the /orders page. So, I think what you want is for when the backend retires the order (e.g. [INF] CORE: Retiring inactive order xxx in status canceled), the frontend can either automatically remove it, or time stamp it for removal after a minute or two.

Note that the retire concept is much harder to observe on simnet because the ticker that does this retiring and removal on the backend fires much much more frequently than it does on mainnet, where the tick rate is based on the "broadcast timeout".

@chappjc
Copy link
Member

chappjc commented Feb 27, 2023

  • it looks like Your Orders list is eventually consistent (meaning, it updates eventually, but might reflect old status for a while - on screenshot below it took ~2 minutes to update settling->executed);

That's not good if it really had finished settling. It used to change status quite promptly.

@chappjc
Copy link
Member

chappjc commented Feb 27, 2023

"Retired" (removed from the trades map on the backend, not just fully executed and settled) orders will never show on the markets page, at least after you refresh the page, because these are no longer tracked order. Such retired orders can only be found on the /orders page. So, I think what you want is for when the backend retires the order (e.g. [INF] CORE: Retiring inactive order xxx in status canceled), the frontend can either automatically remove it, or time stamp it for removal after a minute or two.

For example:

diff --git a/client/webserver/site/src/js/markets.ts b/client/webserver/site/src/js/markets.ts
index 350366e7a..4adc7fade 100644
--- a/client/webserver/site/src/js/markets.ts
+++ b/client/webserver/site/src/js/markets.ts
@@ -2050,6 +2050,10 @@ export default class MarketsPage extends BasePage {
     mord.ord = order
     if (note.topic === 'MissedCancel') Doc.show(mord.details.cancelBttn)
     if (order.filled === order.qty) Doc.hide(mord.details.cancelBttn)
+    if (note.topic === 'OrderRetired') {
+      delete this.metaOrders[order.id]
+      mord.div.remove()
+    }
     if (app().canAccelerateOrder(order)) Doc.show(mord.details.accelerateBttn)
     else Doc.hide(mord.details.accelerateBttn)
     this.updateMetaOrder(mord)

I just tested that and it works as expected. On mainnet with a 22m broadcast timeout and thus a 2m45s tick interval, an order that has become inactive will be retired and now removed from the "Your Orders" table of the markets page. This tick interval is 7.5 seconds on simnet, so it's going to be unrealistically fast however with simnet.

@norwnd
Copy link
Contributor

norwnd commented Feb 28, 2023

So, I think what you want is for when the backend retires the order (e.g. [INF] CORE: Retiring inactive order xxx in status canceled), the frontend can either automatically remove it, or time stamp it for removal after a minute or two.

just to clarify the main point I was trying to get across, I don't think we'd want to force the user to manually clean up "Your Orders" section,

since like you discussed in #2179 page refresh will clean it up anyway this list is unlikely to grow too much so that the user would want to clean it up manually; and so I'm kinda struggling to see a plausible case for brush button existence ... could you guys elaborate on what I'm missing (just my opinion, UX-wise its fine to have such functionality as opt-in feature if it doesn't bloat UI too much) ?

@norwnd
Copy link
Contributor

norwnd commented Feb 28, 2023

  • it looks like Your Orders list is eventually consistent (meaning, it updates eventually, but might reflect old status for a while - on screenshot below it took ~2 minutes to update settling->executed);

That's not good if it really had finished settling. It used to change status quite promptly.

I've captured some of those delays I mentioned, let me know whether anything seems off there,

things to note:

  • I'm doing this on testnet with -race build
  • you can see on /markets page I have 2 orders executing against each other, I have both of them I've opened in separate browser tabs before starting this demo, but I don't refresh browser pages during this demo (just switching between pages to see whether there are any data discrepancies)
  • you can see that Maker order has its status executed at the very start of the demo (I'm not sure whether I opened Maker page and it already had executed status even before I started recording - meaning it could have changed to executed upon page-open - or whether it just changed without any page-refreshes at all)
  • you can also see that Taker is still in settling, and so are both order-views on /markets page until 1m+ into the demo
delayed-order-status-ui-updates.webm

attaching relevant logs, for good measure: order-status-updates-ui-delays.txt

@chappjc
Copy link
Member

chappjc commented Feb 28, 2023

Thanks for that. Let's look to see if there's a missing notification (emit from backend or handle on frontend) for the Redemption Confirmed event.

@chappjc
Copy link
Member

chappjc commented Jun 12, 2023

I noticed the orders come back rather easily, and the sweeper button for all the orders disappears. They all come back if you place an order, or go to a different market and come back.

orderscomebackagain.mp4

@buck54321 What would you like to do with this PR? The sweep doesn't really work as intended since the orders that aren't retired just come back in response to a few different events.

In #2171 (comment), I suggested "what you want is for when the backend retires the order (e.g. [INF] CORE: Retiring inactive order xxx in status canceled), the frontend can either automatically remove it, or time stamp it for removal after a minute or two." Also the frontend could only show the sweep icon for orders for which an OrderRetired topic note was received.

Also, just a reminder that this is very hard to observe on simnet where the tick rate is super fast.

@buck54321
Copy link
Member Author

Obviously the sweep function is bugged. @chappjc and @norwnd seem in favor of automatic cleanup, but I strongly object to these elements disappearing without user interaction.
How about I drop the sweeper button and have this table be (kinda) constant size, showing the max(n, liveOrderCount) most recent orders, for some reasonable value of n. Maybe 5 or 10. That's most recent orders, not live orders, so orders will not disappear without the user placing an order, which adds a row to the top and (potentially) deletes a row from the bottom. This is closer to what I see on CEXs.

@chappjc
Copy link
Member

chappjc commented Jun 20, 2023

Obviously the sweep function is bugged. @chappjc and @norwnd seem in favor of automatic cleanup, but I strongly object to these elements disappearing without user interaction.

I agree with your point about that. Things shouldn't disappear unexpectedly.

How about I drop the sweeper button and have this table be (kinda) constant size, showing the max(n, liveOrderCount) most recent orders, for some reasonable value of n. Maybe 5 or 10. That's most recent orders, not live orders, so orders will not disappear without the user placing an order, which adds a row to the top and (potentially) deletes a row from the bottom. This is closer to what I see on CEXs.

That seems reasonable. Drop completed orders if the space is needed for a new one.

An orthogonal approach that I suggested in #2171 (comment), is to have the frontend show the sweep icon once it receives an OrderRetired note, which means it's not going to come back on refresh or some other update that rebuilds the orders table.

@buck54321
Copy link
Member Author

Some style changes in latest incarnation

image

Allow sweeping of fully-executed orders from active orders list
on markets view. Shows a broom icon in the button menu.

Show (a copy of) the button menu on hover, allowing the user to
perform actions without expanding the active order entry.
@buck54321
Copy link
Member Author

I'll plan to merge this tomorrow.

@buck54321 buck54321 merged commit 883c935 into decred:master Sep 2, 2023
5 checks passed
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.

None yet

4 participants