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

Options to Move ItemLevel text #496

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

legowxelab2z8
Copy link
Contributor

@legowxelab2z8 legowxelab2z8 commented Oct 18, 2020

Fixes #303.
This is another implementation of pull requests #178 and #206.

Changes the default location of ItemLevel text and adds options for changing text anchor and offsets.

Using the options to change the text back to the old position:
Location Restored
The new default location (note that the upgrade icon on the item with an item level 23 is now visible):
New Location

As noted in the bug reports and other pull requests this change mainly focuses on moving the Item Level text so that it doesn't overlap with the UpgradeIcon. I feel this change has merit on its own even for users that do not use the UpgradeIcon. The ItemLevel plugin does one thing; Display the item level of items in your bag. Having an option of where to display that text seems reasonable.

How this is different from #178
The options allow for setting the text to any location for those who do not want it in the bottom left.
This pull will require localization and #178 does not.
Adds 77 lines of code to allow users to change the behavior of 1 line of code.

How this is different from #206
This implementation does not touch UpgradeIcon and only modifies the behavior of the ItemLevel plugin.
In addition to allowing the anchor to be set the offsets can also be configured.
Implementation of the repositioning is done in a separate function that is only called when the position values change and not in the UpdateButton handler.

Added options to ItemLevel module to let the user define an anchor and offset for the item level text.
Fixes the trailing whitespace that cause the build check to fail https://travis-ci.org/github/AdiAddons/AdiBags/builds/736756255
@Talyrius
Copy link
Contributor

Yes, this is much closer to what I had in mind after initially looking over the proposed changes in #206. I was short on time, so I didn't offer an explanation or code review as to why I wasn't accepting it. However, the biggest reason were the upgrade icon changes.

I had planned to author a similar commit that also included font and size options (as implemented in the Skin and Currency menus, minus the color option) in favor of leaving it hard-coded, but I never got around to doing it.

For localizations, the anchor values can do without them and the offsets can just reuse the below strings from Bartender4 as they've already been localized there. The others will need placeholders in Localization.lua as UpdateLocalization.php is no longer being maintained.

L["Anchor"]
L["X Offset"]
L["Offset in X direction (horizontal) from the given anchor point."]
L["Y Offset"]
L["Offset in Y direction (vertical) from the given anchor point."]

@legowxelab2z8
Copy link
Contributor Author

I tried adding font and size options, but it seems that the font options are not being preserved. The Item Level text usually uses the font NumberFontNormal which inherits NumberFont_Outline_Med which as the name implies has an outline. When setting the font from the options UI the outline option is not preserved. This is especially important since the text is on top of an image and without an outline the text becomes hard to read.

Before:
TextOutline

After:
TextWithoutOutline

I will need to investigate further to see how I can get the outline to show. I will work on those localization changes in the meantime.

Added localized strings for 
"Anchor"
"X Offset"
"Y Offset"
"Offset in X direction (horizontal) from the given anchor point."
"Offset in Y direction (vertical) from the given anchor point."
Added an options control for setting the Font, Size, and Color.
Color can be set when the Color Scheme is set to manual
@legowxelab2z8
Copy link
Contributor Author

Added the font options like the Currency plugin uses. There are a couple of workarounds I had to implement to get the font working the way I think it should.

I used the SettingHook to apply the outline option after the font has been changed since the font object from Fonts.lua doesnt support font options and proto:ApplySettings() wipes the font options when calling SetFont.

The font size option from CreateFontOptions has a hardcoded stepsize of 4 which can make the item level text too small or too big. I had to alter the settings table to set the stepsize to 1.

Since the text color gets changed in the update handler (based on the color scheme) the text no longer inherits the font color from the font object so I had to add an UpdateFont function that gets called from the SettingHook to update the font color on all the item level fontstrings

FontDefault

Using the options to change the font size and color (color can be set when the Color scheme is set to manual)
FontCustom

Changed order values for the options table to avoid conflicts with other pull requests.
@Alyred
Copy link

Alyred commented Oct 20, 2020

Can't wait to see this merged in.
You're dropping the upgrade arrow configurability because it should be handled elsewhere? I did like the ability to move it in one of the previous pulls. Any chance of adding it back in with a different pull?

@legowxelab2z8
Copy link
Contributor Author

I didn't like the idea of the ItemLevel plugin being the location for the option to move the upgrade arrow. Perhaps another plugin could handle that or since it is part of the button either the button class itself or the main options should have that change. The problem I had with the pull request that added that option was that it was setting the location in the update handler. I think the position only needs to be updated when the setting is changed or when a new button is made. I haven't looked into it, but there was also some issue with showing upgrade icons for bank items. The other pull request was making a new upgradeIcon to handle that and I don't know if that is still needed or not.

@legowxelab2z8
Copy link
Contributor Author

From a user perspective I like having the option under items where the other options are like the quest indicator and quality highlight.

UpgradeIconOptions

I'll need to do a bit of code cleanup and then I can submit a pull request for the Upgrade Icon position options.

Adibags handles profile changes by calling Disable then Enable. Use OnEnable instead of using new callback handlers.
Fixed so font should now properly load. Since the fix is in OnEnable this will also fix font loading when changing profiles.
@Alyred
Copy link

Alyred commented Nov 22, 2020

I've been testing these commits for some time, and just re-integrated them into the current version, and they seem to work flawlessly.

This was referenced Dec 3, 2020
@griddark
Copy link

I'm using this on a version of adibags that's skinned for elvui, I intigrated everything and it works, my only issue is that the text set's itself to the default location and changes after I move it using the x/y option to the right place.

Copy link

@Alyred Alyred left a comment

Choose a reason for hiding this comment

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

All code still works with 1.9.23-3alpha, but needs to be manually integrated due to some code changes and movement.

@Beet4
Copy link

Beet4 commented Nov 29, 2022

Any updates on integrating this into AdiBags? It would be awesome as currently the ilvl text covers the Pawn upgrade button.

Edit: Not a huge issue anymore since AdiBags 1.9.49 moved the itemlevel text to the bottom left instead.

@juntereiner
Copy link

I would love to see this merged because ilvl text position conflicts with other addons and I need to manually change the position each time there is an update.
Thanks.

@Sattva-108
Copy link

What is 'sorting' options table for?
I implemented this pull request in my legacy version of AdiBags, and the 'sorting' table seems to cause Ace Options DB error, for new users, with empty AdiBags SavedVariables.
I just deleted 'sorting', seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suggestion: ilevel module - show numbers at bottom of the item
7 participants