Fixed Console Output Stall & General QoL Changes#685
Fixed Console Output Stall & General QoL Changes#685Knewest wants to merge 6 commits intoatampy25:mainfrom
Conversation
|
|
||
| {#if userHasScrolledDeployOutput} | ||
| <button | ||
| class="absolute top-[288px] z-10 right-[37px] bg-gray-400 hover:bg-red-500 text-white py-1 px-3 rounded" |
There was a problem hiding this comment.
Do these absolute styles work the same across the common resolutions (720p, 1080p, 1440p, 2160p)? You can test using responsive mode in DevTools if you haven't already
| {#if warnings.length > 0 || errors.length > 0} | ||
| <br /> | ||
| <div id="notificationElement" | ||
| class="flex flex-row gap-2 flex-wrap max-h-[25vh] h-[126px] overflow-y-auto relative" |
There was a problem hiding this comment.
Same thing as before with this absolute height value; usually Tailwind values are preferable as well (h-40 or something; Tailwind autocomplete will tell you the pixel values)
|
|
||
| {#if userHasScrolledNotifications} | ||
| <button | ||
| class="fixed top-[479px] z-10 right-[38px] bg-gray-400 hover:bg-red-500 text-white py-1 px-3 rounded" |
| {/if} | ||
|
|
||
| <div class="inline absolute right-5 text-right"> | ||
| {#if totalNotifications > 0} |
There was a problem hiding this comment.
I would prefer directly using the two length values for this rather than 3 computed variables
| background-color: #262626; | ||
| } | ||
|
|
||
| .overflow-y-auto { |
There was a problem hiding this comment.
Use Tailwind (or inline styles if there is no Tailwind class for colour scheme) for this; overriding a style like this may override it globally across all pages which is highly inadvisable.
|
|
||
| :global(.bx--modal-header) { | ||
| margin-bottom:0rem !important; | ||
| } |
There was a problem hiding this comment.
Same thing with all these global styles for the modal classes. For the inline notification, it may be best to copy the final HTML elements and style those individually instead of using the component given how many changes are being made).
|
|
||
| {#each warnings as warning} | ||
| <InlineNotification hideCloseButton lowContrast kind="warning"> | ||
| <div slot="title" class="-mt-1 text-lg">Warning #{warning.count}</div> |
There was a problem hiding this comment.
Use warnings.entries() as [index, warning] rather than storing a count variable for each
| warnings = []; | ||
| errors = []; | ||
|
|
||
| deployOutput.split(/\r?\n/).forEach(line => { |
There was a problem hiding this comment.
The forEach here would probably be a bit nicer as a for loop since you're not using any other functional programming here
| document.getElementById("deployOutputElement")?.children[0].scrollIntoView(false) | ||
| }, 100) | ||
| }, 500) | ||
| function enableAutoScrollDeployOutput() { |
There was a problem hiding this comment.
These sorts of very small functions can be written inline as () => {} arrow functions in the Svelte on:click handler
| deployOutputHTML = convertAnsi.toHtml(deployOutput) | ||
| const element = event.target; | ||
| const nearBottom = element.scrollHeight - element.clientHeight <= element.scrollTop + 10; | ||
| // The VSC error is right, so do not add a non-null assertion for it. If you do, the user will not be able to scroll in the deploy and notification output anymore. - Knew |
There was a problem hiding this comment.
What does this mean? Non-null assertions don't change runtime behaviour.
|
Would be great if this was pulled in, the GUI output could really benefit from some (margin) resizing options and some formatting/extended logging to debug which mods are taking longer than usual to merge in (such as a runtime indicator for the patches and maybe extended warning on the specific lines being patched in that are causing the program to sit for 10m on one patch) and maybe grouped/collapsible error messages. I don't know how much of this pull has been completed but if bandwidth is low I'd be happy to assist in some of those things being made. Hitman already crashed on its own accord so debugging crashes from mods is proving tricky. |
|
None of the comments have been addressed so the PR isn't mergeable. Regardless, SMF v2 is not receiving updates aside from fixes as development is focused on version 3, whose status you can review in the pinned issue. |
3 participants
Overview:
Hello, this pull request introduces significant improvements to the console and notification output system within SMF. The primary goal is to address and resolve the performance bottleneck caused by frequent DOM updates during the mod deployment process. I've identified that the current output rendering approach can significantly slow down mod deployment artificially, particularly when dealing with a high volume of warning messages. My proposed changes not only optimise performance but also enhance the user interface, making it more user friendly and informative.
Key Enhancements:
Fixes and Improvements:
Video/Image Of Changes:
DldFJLsJaU.webm