DanielScholte / GuildWars2Companion

GW2 Companion is an unofficial open-source Guild Wars 2 app. GW2 Companion helps you keep track of your account progression and characters, and provides information to help you on your journey in Tyria.
GNU General Public License v3.0
63 stars 14 forks source link

Item flags retrieval and display #55

Closed apcro closed 3 years ago

apcro commented 3 years ago

Description

When getting item details from the API, also retrieve and store the available item flags.

These come as a list of strings, which are converted and stored in the ItemFlags object as booleans, and then stored in the database as Integers. On retrieval, the stored Integers are converted back to boolean for use in item display.

An example of extending item information display has been implemented in the Bank view, with Bank items displaying some additional information based on the Flags (such as Account Bound, Soul Bould, Unique etc). If an item being view has additional flags from the configured list, they will be displayed underneath a divider.

How Has This Been Tested?

Tested in-app using a real player account, viewing the additional information displayed.

Notes

Dart in Flutter doesn't support reflection, so the ItemFlags object is generated manually based on the list of possible strings returned by the API.

apcro commented 3 years ago
  • The API returns the flags in a string array. I'd recommend to convert this string array to a string list, instead of to multiple booleans. You can store this string list inside of the Item class instead of a seperate class.
if (json['flags'] != null {
    this.flags = json['flags'].cast<String>();
}
  • You can then also save the flags as a TEXT inside the database, which can be the String list joined with semicolons.
flags TEXT

I'll modify the code to do it this way, but retain the boolean structures for now - discussion below :)

  • Also keep in mind that you shouldn't edit the _initializationScripts of a migration. Instead, add a new query under _migrationScripts where you add the additional column.

I'll update this - this is a learning piece for me.

  • Lastly, on the item page, you can do something like this:

I'll implement the flags code in this way, however I think this may miss context when displaying flags.

Discussion: Any item retrieved could have multiple flags, and the context for displaying information I think is important rather than just displaying all flags blindly.

One of my test objects, "Communal Boost Bonfire" (id 41741) has multiple flags not all of which may be useful information in all contexts: accountBindOnUse, deleteWarning, noMysticForge, noSalvage, noSell, noUnderwater.

In the context of the Bank tab, it's probably not be useful to show deleteWarning and noUnderwater. In the Bank context as well, despite accountBound not being set, if an accountBindOnUse flag is true and the item is in the bank, then it's likely also account bound so displaying that message rather than the 'bound on use' message might be clearer.

In another example, the item "Jurah's Jewel" has the flags hideSuffix and notUpgradeable, which again might not be useful information in the context of the Bank, but may be useful in other item contexts (for example, notUpgradeable would be useful information on the Equipment page.)

For soul bound items, the flag soulBindOnUse would translate as a straight 'Soul bound' message if the item was in the character's Equipment roster, but not necessarily if it was in the Bank

DanielScholte commented 3 years ago

Discussion: Any item retrieved could have multiple flags, and the context for displaying information I think is important rather than just displaying all flags blindly.

One of my test objects, "Communal Boost Bonfire" (id 41741) has multiple flags not all of which may be useful information in all contexts: accountBindOnUse, deleteWarning, noMysticForge, noSalvage, noSell, noUnderwater.

In the context of the Bank tab, it's probably not be useful to show deleteWarning and noUnderwater. In the Bank context as well, despite accountBound not being set, if an accountBindOnUse flag is true and the item is in the bank, then it's likely also account bound so displaying that message rather than the 'bound on use' message might be clearer.

In another example, the item "Jurah's Jewel" has the flags hideSuffix and notUpgradeable, which again might not be useful information in the context of the Bank, but may be useful in other item contexts (for example, notUpgradeable would be useful information on the Equipment page.)

For soul bound items, the flag soulBindOnUse would translate as a straight 'Soul bound' message if the item was in the character's Equipment roster, but not necessarily if it was in the Bank

I get where you're coming from, but that should still be possible if you're using a list of string instead of a few booleans. The issue I have with the booleans is that the code quality isn't very good. It requires a lot more code than is necessary, and it requires you to add new booleans when new flags are added to the game.

What you could do, is create a function which gets a few context parameters like if the flag is even present, from where the item was opened and what the item type is. Then in the function you can decide whether or not to display the flag. In regards to the differences between using booleans and the list of strings, the difference would be:

if (item.flags.soulBindOnUse) // Booleans
if (item.flags.contains('soulBindOnUse')) // List of strings

An alternative would be to use enums instead of string. This way it should still reduce the amount of code significantly, but might be a good middle ground.

apcro commented 3 years ago

I've made some changes initially just to get, store & display item flags (except HideSuffix, I'm unclear what benefit knowing this has). I'll look to controlling what's displayed where in another commit.

apcro commented 3 years ago

I've modified CompanionItemBox to allow passing a section parameter (which defaults to 'all'), allowing later manipulation of the retrieved flags (and their cache states, which appear to differ sometimes especially around Account Bound and Soul Bound flags).

And in a change I left out of the commit message, I modified general/item/item.dart to left-align the item name, and wrap the text if it overflows using Expanded (L#120)

For example: Screenshot 2020-09-16 at 12 05 51

apcro commented 3 years ago

Added a few comments regarding the changes. Furthermore, there are currently two bugs, not caused by your changes, but will affect them:

  • The app currently uses sqflite_migration: ^0.1.2. There is a bug in this version that will cause crashes when updating from an older version of the app. This has been fixed in a newer version, and therefore we should switch to this newer version, ^2.0.0

  • Because flags are currently not saved in the db, current users will only get to view the flags when they empty their cache. Something will have to be implemented to empty the cache if the structure of the object changes.

I thought I'd caught all the places where it was supposed to save the flags in the item table, but on reflection since I added to the existing table, for those who already have the item in their cache then yes, this wouldn't be updated.

And thanks for clearing both of these up for me.

apcro commented 3 years ago

First of all, my apologies for the late response. I've added a few small last remarks regarding the UI and some of the pieces of code. After that it should be fine, and i'll merge it into development. I'll be releasing it in version 1.6, along side some of my own features and the bug fixes I talked about earlier. I'm quite busy at the moment, so please keep in mind that it might be a little while before this will be released.

I've made the last few changes now.

I'm not concerned about time :) I'm slowly working on some of the other issues (or rather trying to work out if I can solve some of the other issues as a learning process) going back and forth with different approaches and potential solutions, so whenever it's released I'll be happy.

DanielScholte commented 3 years ago

Sounds great 😃 Keep in mind that it would be helpful if you could reply to the issues that you're working on, or atleast reply to them once you've made significant progress. This is to prevent multiple people working on the same issue.

apcro commented 3 years ago

Yes, this is my intention when I get to that stage. At the moment I've been looking at the Backstory issue and the Minis issue, but I'm still working my way through how the overall codebase works. I've tried a number of different approaches, none of which have worked properly and all of which I've reverted entirely, so I'm definitely not ready to comment on the issue solutions, and will be more than happy if someone else looks at them before me.