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

CWindowWithArtifacts refactoring part2 #3895

Merged

Conversation

SoundSSGood
Copy link
Contributor

@SoundSSGood SoundSSGood commented May 6, 2024

  • CWindowWithArtifacts - refactoring. Reducing code complexity.
  • ctrl+click; alt+click now works in the backpack window.
  • ctrl+click now works in the artifact altar window.
  • ArtifactPosition::TRANSITION_POS is now simple ArtSlotInfo. Not a vector.
  • Shift+click on the artifact slot should open the quick backpack window.

@SoundSSGood SoundSSGood force-pushed the CWindowWithArtifacts-refactoring2 branch 5 times, most recently from 42dca77 to 2079281 Compare May 8, 2024 18:14
@SoundSSGood SoundSSGood marked this pull request as ready for review May 8, 2024 18:57
@dydzio0614
Copy link
Member

why was shift + click idea abandoned? it would be good feature in addition to having this at mouse wheel

@SoundSSGood
Copy link
Contributor Author

why was shift + click idea abandoned? it would be good feature in addition to having this at mouse wheel

There is no way to close the window without swapping artifact.

@dydzio0614
Copy link
Member

there is backpack button at the bottom of hero window - maybe that variant of backpack could be reused in this case?

@SoundSSGood
Copy link
Contributor Author

Maybe yes. I'll deal with this a little later

@SoundSSGood SoundSSGood force-pushed the CWindowWithArtifacts-refactoring2 branch 3 times, most recently from f93caaa to df91317 Compare May 17, 2024 12:55
client/widgets/CArtifactsOfHeroMarket.cpp Outdated Show resolved Hide resolved
client/widgets/CArtifactsOfHeroMarket.h Outdated Show resolved Hide resolved
Comment on lines 22 to 28
using ArtifactsOfHeroVar = std::variant<
std::shared_ptr<CArtifactsOfHeroMarket>,
std::shared_ptr<CArtifactsOfHeroAltar>,
std::shared_ptr<CArtifactsOfHeroKingdom>,
std::shared_ptr<CArtifactsOfHeroMain>,
std::shared_ptr<CArtifactsOfHeroBackpack>,
std::shared_ptr<CArtifactsOfHeroQuickBackpack>>;
Copy link
Member

Choose a reason for hiding this comment

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

I know that this is not a new code, but why you opted to use variant here instead of using interface with different implementations with virtual methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At a certain stage of working with the code, it was easier to distinguish which window we were working with at the moment. But for now, you may be right. Switching to virtual methods may make the code even simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to redo it

@SoundSSGood SoundSSGood marked this pull request as draft May 20, 2024 09:36
@SoundSSGood SoundSSGood force-pushed the CWindowWithArtifacts-refactoring2 branch 4 times, most recently from 5e0bb36 to a5ca274 Compare May 20, 2024 21:53
@SoundSSGood SoundSSGood force-pushed the CWindowWithArtifacts-refactoring2 branch from a5ca274 to 3692ca2 Compare May 21, 2024 09:05
@SoundSSGood SoundSSGood marked this pull request as ready for review May 21, 2024 09:45
@IvanSavenko IvanSavenko merged commit 9e0bae9 into vcmi:develop May 30, 2024
14 checks passed
@SoundSSGood SoundSSGood deleted the CWindowWithArtifacts-refactoring2 branch May 31, 2024 08:16
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

3 participants