CustomBangSearch

CustomBangSearch copied to clipboard

These two commits should fix the issue #11 and also improve the way potential duplicates in the registered bangs are highlighted.

Thanks for the PR!

This is good, but it still has some issues:

• If you click in a bang, don't edit it, then click out, it will show the "cant rename to currently used" warning, I guess we would need to check if the bang has actually changed from before being focused, before running the if (newBang in bangs) check
• If you make an invalid change to a bang, leaving the box red, then make a valid change to another bang, it highlights the success button and allows you to "save" the bangs. This isn't the end of the world as it doesn't actually end up saving the invalid bang, but could be confusing for some users?

There's potentially other issues with it as well, I found these 2 after a few seconds of playing around with it.

I really think there are only 2 solutions to this problem, both of which will require a lot of changes, either:

• The structure of the stored bangs is changed so that the bang id is the key in the object instead of the actual bang. This will introduce yet another legacy format of stored bangs which will have to be accounted for and converted on the fly, and also require a lot (I think) of the React code changing, or
• We load the saved bangs into an intermediate structure (probably the one I just described), use that in the React, and then when we save, convert back to the save format. This would maybe be easier but would also introduce a lot more complexity and is really just a hack rather than a solution

If you have any ideas or want to update your PR I'd be happy to test it out :)

Indeed, there were some errors with the previous solution. As you can see, I've added some more changes that should address all of the unwanted side-effects. One big part is that the save button is now being disabled when there are no changes - therefore we can have full control over when the button can be clicked. The aforementioned case with an invalid bang that is correctly not being saved but might still be confusing to the user can therefore be avoided.

I only found two issues with the proposed solution:

• when changes are made, they are registered, but when they are undone, they are not unregistered (the unsavedChanges flag is not reset - the save button is not disabled again)
  • this was previously also no the case, which is why I think that is ok for now, but should be addressed in the future
• when bang "a" is changed to "ab" while there is already a bang with the name "ab", and then the original "ab" bang is changed, the save button lights (correctly) up (as there are no more conflicts), but the error in the (not anymore invalid) field is not reset, as it's not aware that there are no more conflicts.
  • is in my opinion an edge case and is only of cosmetic nature, therefore should be addressed in a future update (this requires indeed a more thorough refactoring of the state-management in my opinion) but should not be a show-stopper as the proposed changes still fix a quite big bug

Though I've tested this time way more (and also read more of the code to check if it breaks something else), don't hesitate to check the changes and leave some comments.

Hey sorry I've been real busy recently, I haven't forgotten about this but it might take me some time to test & merge it if its good :)

Apologies this took so long.

I've just tested your changes and everything looks good, yeah I agree with your comments, and yes I think it's fine for now, even if there are a edge case or two

Just to document it, there's also another slightly weird edge case issue where sometimes the save button stays disabled when it shouldn't; for example if you have the default bangs, rename e to a, click out the box so it registers the error, then change the red a to another different unused single letter, such as f. The red goes away but the save button does not highlight. This seems to happen inconsistently and I'm unsure why, sometimes just when changing a bang normally. It's fine for now but probably not a great bug to have.

To be honest I'm unhappy with the quality of my TS/React (not yours, just the whole options page code in general) I think it's quite messy and difficult to track. It's fine for now but I am considering rewriting the settings page at some point, it really shouldn't be this spaghetti for such a simple settings page (again, not saying anything about your code, it's my code I'm unhappy with :) )

Thanks again for these fixes, feel free to contribute more if you want to!