Developing Bugs

[drdev]

@Dan, r29237: you added a lot of switch-case statements to RandomDeckGenerator that my IDE picks out because they're all falling through, ie. no breaks. Is this supposed to happen? Judging from the code I'd say no...

Fixed in r29257.

[elcnesh]

Hey all! I've just made a rather large commit (r29317) with a huge cleanup of the gui and gui-desktop code. I've removed several unused classes and methods (as well fixing some bugs I encountered while cleaning – which is actually why it's useful in the first place). If there's anything I removed that you were using in your workspace, please mention it! I can easily restore stuff. This commit isn't meant to remove useful things, only to get rid of some very old parts of the code that weren't being used anymore. Cheers!

Edit: I just saw the log exceeds the 33MB limit of the WebSVN viewer. I hope this doesn't break anything

[drdev]

Hey all! I've just made a rather large commit (r29317) with a huge cleanup of the gui and gui-desktop code. I've removed several unused classes and methods (as well fixing some bugs I encountered while cleaning – which is actually why it's useful in the first place). If there's anything I removed that you were using in your workspace, please mention it! I can easily restore stuff. This commit isn't meant to remove useful things, only to get rid of some very old parts of the code that weren't being used anymore. Cheers!

Edit: I just saw the log exceeds the 33MB limit of the WebSVN viewer. I hope this doesn't break anything

Are you sure they weren't being used by the mobile app?

[elcnesh]

Yeah, I ran a few games on it for testing

[KrazyTheFox]

Edit: I just saw the log exceeds the 33MB limit of the WebSVN viewer. I hope this doesn't break anything

That is one huge diff. But I've got you covered: https://gist.github.com/KrazyTheFox/c59bda9506f096f8c53b and https://gist.github.com/KrazyTheFox/9e208e2eced719f4da8f

I think for clarity switch statements should be indented, but otherwise everything looks good and you haven't broken much that I was working on in the process. Nicely done.

[Agetian]

In r29402 I reverted the commit that was meant to remove "duplicate" Vanguard avatar definitions. The duplicates are indeed correct, each of the duplicated avatars in Vanguard.txt had two distinct variations (with two distinct pictures, standard and alternate art). Gatherer lists them as cards with two variations, and card picture packages exist that have both art variations. Those packages do not work correctly with the duplicates removed.

- Agetian

[Agetian]

r29403: I attempted to fix the issue with morphed cards leaking their color in CardDetailPanel border. However, looking at the related code I figured out that there may be a bigger problem there: canShow is always set to 'true' on line 199, but a lot of further code seems to assume that canShow may be either true or false... I don't know if this is intentional or not, anybody who feels comfortable with the GUI code, feel free to take a better look at this code...

- Agetian

[KrazyTheFox]

r29403: I attempted to fix the issue with morphed cards leaking their color in CardDetailPanel border. However, looking at the related code I figured out that there may be a bigger problem there: canShow is always set to 'true' on line 199, but a lot of further code seems to assume that canShow may be either true or false... I don't know if this is intentional or not, anybody who feels comfortable with the GUI code, feel free to take a better look at this code...

- Agetian

I looked at the history and the commit that changed this is r28874 (Link shortener used to actually allow linking). MatchUtil.java was removed and doesn't appear to have been replaced here.

[Agetian]

Thanks for the tip, Krazy! I'm committing a (hopefully) better fix, probably not ideal either, but seems to do its job fine and actually tries to make canShow useful when explicitly called with a parameter from some context that can work directly with MatchUI and obtain information about whether a card can be viewed by a particular player or not [r29405]. Feel free to expand on this if necessary.

EDIT: Ok, so my previous idea in r29405 didn't work - it hid the morph color in some cases but not in some other cases (and I couldn't figure out why). This is complicated by the fact that CMatchUI is not accessible from CardDetailPanel and there's no equivalent for the MatchController class that is used in forge-gui-mobile for this purpose, so the implementation most likely has to be done elsewhere or has to be more complicated. I'm not very good at this portion of Forge, so feel free to implement this in a better way if you know how - for now I'll resort to the hackier but working version of the fix that hard-sets the border color visibility for face down cards.

- Agetian

[elcnesh]

This part is all my fault. I removed MatchUtil because it required one static Game instance, which was rather incompatible with network play. It's been replaced one-to-one with HostedMatch. canShow is always true now probably because I figured it'd be the responsibility of CardView to set the card's properties correctly in case the card can't be viewed, but I agree the code is rather weird here. I'll have a look into it
Thanks for finding this by the way, I've looked into it already but couldn't find it in CardView. The bug is probably here!

[Agetian]

This part is all my fault. I removed MatchUtil because it required one static Game instance, which was rather incompatible with network play. It's been replaced one-to-one with HostedMatch. canShow is always true now probably because I figured it'd be the responsibility of CardView to set the card's properties correctly in case the card can't be viewed, but I agree the code is rather weird here. I'll have a look into it
Thanks for finding this by the way, I've looked into it already but couldn't find it in CardView. The bug is probably here!

Ahh, makes sense! Hopefully you'll be able to eventually devise a proper solution for it. Good luck!

By the way, the way it is there's currently an even bigger related bug that basically allows cheating: if your opponent casts a Morph creature, it's possible to see what that creature is (picture and all) by activating the card zoomer and then using the "Flip" feature (I assume it should be disabled for the cards you don't own/control, but for whatever reason it isn't). It looks like the reason it's so is that CardZoomer has no way of telling whether it should disable or enable the flip indicator (in CardZoomer.setFlipIndicator) - once again, I don't think I'm the best person for handling this, so your help would be more than welcome!

- Agetian

[Agetian]

Also, I ran into some issues with r29415: since there are hundreds of thousands of net decks now available for Forge it can be a bit of a pain for an ordinary user (who does not know how to make some kind of a batch script to automate the process) to fix the decks in a way such that they contain those new accented symbols. Is it possible, in any way, to make the game detect these card names both ways when loading decks (special character or not), showing pictures, and asking the user to type card names?. Kind of like making those symbols equivalent to their non-accented counterparts. EDIT: I tried to implement this in r29417, but my implementation may not be efficient (it depends on how often reIndex needs to be actually called... I know it's called when the cards are read from disk and CardDb is initially compiled but it looks like it may also be called under some other circumstances, please take a look at the code...).

P.S. I'm also not sure how to type those symbols, so if, let's say, I want to find Deja Vu by typing its name in the card search dialog box, this complicates my search experience (I can only type "D" and then I have to manually scroll down because the accented "e" is not detected if I type a simple "e").

P.P.S. Another issue with it is that it doesn't show correct pictures for those cards anymore (because CCGHQ card packs have unaccented names for these cards). EDIT: Looks like I fixed this one in r29416 by changing the file encoding.

P.P.P.S. And one more issue, this time with the dev environment: I can't open ImageUtil.java in NetBeans now, it says "this file can't be safely opened with the UTF-8 encoding", and if I proceed it opens it but it shows all those accented symbols as question marks... Is the source file really saved in UTF-8? :/ Also, when compiling Forge I get a lot of warnings, I think these warnings hint that the relevant code is actually not being treated properly when the game is compiled - EDIT: Looks like I fixed this one in r29416 by changing the file encoding.

Code: Select all

Compiling 115 source files to /home/agetian/NetBeansProjects/trunk/forge-core/target/classes
forge/util/ImageUtil.java:[110,48] unmappable character for encoding UTF-8
.......


P.P.P.P.S. After reading ImageUtil.java in Vim and checking its file encoding I see that it's set to "latin-1" instead of to "UTF-8"... Something's wrong with that for sure, I think... - EDIT: Looks like I fixed this one in r29416 by changing the file encoding.

- Agetian

[elcnesh]

Wow, I had no idea this change would cause so much trouble... I had it staged for a while and played a lot with it succesfully. I'm glad you managed to fix some things already. I'll take a look at your changes and see if there anything to do to make it more efficient, but we can also just revert my commit, it's not that big of a feature and definitely not worth this much trouble

[Agetian]

r29421: I fixed the lookup code to work with set info and art index info in card information, it looks like it works correctly now. The only thing I don't know how to implement is how to implement card search with those weird accented symbols. While it's a relatively minor nuisance, it can still prove to be problematic at times (for cards such as Deja Vu that have the second symbol accented, so one will have to scroll down from the letter D to find it, or Lim-Dul's Whatever since there are several cards called that way and one can typically only search for Lim-D and then will have to scroll manually to the required card).

This is probably the best I can do with the UTF-8 code. Feel free to expand or fix. If this functionality proves not to work correctly or be too much of a nuisance, it may be possible to revert it (in that case it'll have to be reverted altogether with my changes - r29415-r29422).

EDIT: Tested the card search a little bit, currently searching for cards with accented characters can be very difficult. For instance, Deja Vu is located in the very end of the letter D (after "Dwarven....."), which is not only counter-intuitive but also difficult to get to if you search for the letter D first. Actually, it's easier to search for E and then scroll up a bit...

- Agetian

[Agetian]

Oh well, I seem to be stumbling across a problem after a problem... It looks like the accented characters also complicate deck editing. For instance, it's very difficult to reach or find cards like Deja Vu or Seance or Marton Stromgald in the deck editor - searching for "de", "se" or "ma" does not work, and looking at the deck list itself does not show e.g. "Seance" along with other "se" card names, instead it's added to the very end of the S list. If your list is not sorted alphabetically then you're pretty much screwed 'cause God only knows where the card might end up being... I *think* it may not be too much of a problem if you have a keyboard input mode that allows to input all those things (and provided that you know which cards are spelled with which accent marks), but I don't really know if there are many people who are like that...

So, what do you think? Should we revert everything until all these issues can be properly addressed?.. I kind of like the idea of having those characters in but in the current implementation, even with all my changes, in certain cases it may be too much trouble for a minor benefit... :/

- Agetian