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: formatting, fixes and improvements #2

Closed
wants to merge 24 commits into from

Conversation

paulbuechner
Copy link

@paulbuechner paulbuechner commented Jan 27, 2023

Hi Mike,

appreciate your work in providing some maintenance love to the project.

This PR includes the following features, fixes, and improvements:

Cheers 😊

fixes Sellorio#177, Sellorio#199

@paulbuechner paulbuechner marked this pull request as ready for review January 27, 2023 21:53
@paulbuechner paulbuechner changed the title feat: introduce formatting feat: introduce formatting, minor improvements Jan 27, 2023
chore: update vscode globals recognition

fix(formatting): unset max line length

- remove tab indents in favor of spaces

chore: update .editorconfig
@paulbuechner paulbuechner changed the title feat: introduce formatting, minor improvements feat: introduce formatting, fixes and improvements Feb 2, 2023
- fix `Mega Macro` title to no longer get hidden behind top frame bar
- adjust size of `Save` and `Cancel` buttons (grow buttons to make use of unused whitespace between them)
- fix `FramePortrait` placement
- fix `Lock/Unlock` button size to correctly match frame border height
- update xml intentation to use spaces (be consistent across other files)
@paulbuechner
Copy link
Author

paulbuechner commented Feb 3, 2023

Do not merge yet... more fixes are coming. Preferably convert it to draft.

Also, I'd love some feedback on this PR, so we can discuss the changes I made. 😊

paulbuechner and others added 8 commits February 4, 2023 03:12
*feat(conditionals): add linting support for new `known` and `unknown` conditionals (Sellorio#199)
*refactor: EventFrame to support fixed update intervalls
*fix: temporarily disable range-based hotkey coloring (Sellorio#176)
*fix: temporarily disable MainMenuBar (ActionBar) tooltips (Sellorio#131, Sellorio#172, Sellorio#173)
*chore: minor improvements
*ci: version bump
…onditionals

*fix: use UTF-8 strlen to determine wordlength

fixes: Sellorio#199
@aurelion314
Copy link
Owner

FYI something is causing some stutter with this branch. I also got some errors during solo shuffle but was too into it to remember what it was heh. I'll keep playing with it but if you have any idea what is causing the CPU usage let me know.

For reference, here is AddonUsage for both version. This branch is using more than twice the CPU, and almost 20mb more memory:
image

Current version:
image

@paulbuechner
Copy link
Author

Thanks for the feedback. I'll definitely have a look soon 👀

@aurelion314
Copy link
Owner

It seems to be related to UTF-8, even though the error I saw actually said it came from WeakAura, but it doesn't do that when I revert to the old version, so I think the UTF-8 change here is being shared and not playing nice with other addons.

@JoeLatte88
Copy link

has anyone had time to dive deeper into the potential UTF-8 issue? I may take a peek to see if I can help, but my exp with these things are very limited.

@aurelion314
Copy link
Owner

Maybe we can turn this into smaller PRs? @paulbuechner or @JoeLatte88. It is difficult to dig into what actually changed since so many files have formatting updates. Combine that with multiple feature changes and it becomes difficult to isolate what is causing the bug and/or the increased CPU load.

Unfortunately I've been busy and not playing WoW much so I am unlikely to dig through this myself in the near future :\

@JoeLatte88
Copy link

I agree on the "smaller PRs"

@JoeLatte88
Copy link

JoeLatte88 commented Mar 26, 2023

@paulbuechner , perhaps we should roll back the UTF-8 changes and deliver the problem fixes for the time being. I'll go ahead and cherry pick UTF-8 changes out in a clone of your branch and verify if performance is back to what we'd expect. At least then, we can push this out to fix issues and then work on optimization for UTF-8

@paulbuechner
Copy link
Author

Hey! I'm on it today... let me check some changes I made to confirm if the issue is really related to UTF-8.

@paulbuechner
Copy link
Author

paulbuechner commented Mar 26, 2023

@aurelion314 @JoeLatte88

Some information I've found:

The massive memory consumption is caused when loading the icons here to make them searchable in the icon browser when creating a macro.

If we disable the feature the icon starts idling around 700k
grafik

Nevertheless, a memory leak/creep is slowly ticking up...

grafik

EDIT: Reading some resources on this topic I think we should be fine with this issue:
grafik

Regarding the massive memory consumption... I propose rewriting the icon browser to only load when creating a macro. Maybe something like WeakAura has, which works like:

  1. Do not load & show any icons in the icon browser until the user enters the spell name. This will prevent us from loading all icons simultaneously -> only load and cache what is requested.
  2. Implement something like use-debounce to make loading the requested icons, even more, memory/CPU efficient.
  3. Invalidate cache if icon browser is closed / macro is created to free up memory.

@JoeLatte88
Copy link

JoeLatte88 commented Mar 30, 2023

@paulbuechner , is the current code is a safe state from a performance perspective for us to review for approval? this is already a pretty big PR hehehe :)

would make sense to push and release what we have and we can work on further optimizations and such in a separate PR to at least get the fixes out

@paulbuechner
Copy link
Author

The current state works well for me. At the moment I've no further changes planned. I gave you the info I figured out about @aurelion314 concerns here if you would like to do further investigations.

Also I've documented any noticeable changes at the top.

@JoeLatte88
Copy link

@aurelion314 , I reviewed the code and a VAST majority of the files changed are just adding spaces or updating tabbed spacing across some files. I don't see an issue with some of the updates from a code perspective which aren't already mentioned in @paulbuechner notes which will go as part of the addon release notes.

@aurelion314
Copy link
Owner

Nice! Unfortunately I stopped playing and don't even have WoW installed to try this out >_>

@paulbuechner do you want to make this PR directly into the original (https://github.com/Sellorio/mega-macro)? Sellorio has to finalize the merge anyway, and I don't want to hold up your updates.

@paulbuechner paulbuechner changed the title feat: introduce formatting, fixes and improvements feat: formatting, fixes and improvements Apr 2, 2023
@paulbuechner
Copy link
Author

Ok. I opened the PR (Sellorio#202).

Let's see how it goes 😃

- TODO: `IconBrowser` migration
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.

Icon not working on some bars
3 participants