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

Refactored VMenu::ShowMenu and its vicinity. #767

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

MKadaner
Copy link
Contributor

@MKadaner MKadaner commented Dec 27, 2023

Summary

Refactoring in preparation for #711.

References

Checklist

  • I have followed the contributing guidelines.
  • I have discussed this with project maintainers:

    If not checked, I accept that this work might be rejected in favor of a different great big ineffable plan.

Details

Sorry, it is large, but I believe overall the code is cleaner and easier to understand.

CodeFactor is complaining, and probably rightfully so. On the positive side, one issue is fixed.

far/vmenu.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@alabuzhev alabuzhev left a comment

Choose a reason for hiding this comment

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

It looks way too complex for a thorough review :)
I trust you spent quite some time debugging it and know what you're doing, so the comments will likely be only about technical details & style.

far/vmenu.hpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
@MKadaner MKadaner force-pushed the mzk/VMenu-ShowMenu-refactoring branch from 05e4c72 to 976304e Compare December 28, 2023 06:02
@MKadaner
Copy link
Contributor Author

[T]he comments will likely be only about technical details & style.

I addressed all comments so far. Could you please check them and resolve whatever looks good?

I trust you spent quite some time debugging it and know what you're doing...

Well, yes, but... Thorough testing is difficult and time consuming.

@rohitab, if by any chance you can spend some time testing this change, I would hugely appreciate it. The change affects all kinds of vertical lists: menus, histories, Find All, comboboxes... The bugs are expected with vertical layout (irregular movement of selection or items, unexpected empty lines, etc.) and with drawing list borders.

I will of course test more during the long weekend. Just now, I noticed one glitch. Find File -> Code Page Combobox may appear something like this (note the mangled left border; if there is no scrollbar, the right border should look similar):

Using code page:    
Automatic detection 
┌───────────────────
├  1201 │ UTF-16 (Bi
├───────┼───────────
├  37   │ IBM EBCDIC
├  500  │ IBM EBCDIC
├  708  │ Arabic - A
├  720  │ Arabic - T
├  737  │ OEM - Gree
│  775  │ OEM - Balt
└───────────────────

I guess I know how to fix this one (even two ways). Will do during the weekend as well.

@MKadaner MKadaner force-pushed the mzk/VMenu-ShowMenu-refactoring branch 2 times, most recently from cb954e7 to 4fb379e Compare December 29, 2023 07:18
@MKadaner
Copy link
Contributor Author

Fixed everything, including the bug with vertical borders.

Not squashing yet to make verification of the comments easier. Also, I want to test a bit more.

Please let me know if everything is OK.

far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
far/vmenu.cpp Outdated Show resolved Hide resolved
@alabuzhev
Copy link
Contributor

The change looks good, although I haven't tested it.

Thorough testing is difficult and time consuming.

Indeed. We're not in a hurry, so if you'd like to have another look or play more with it, please take your time, we can get back to it next year :)

@MKadaner MKadaner force-pushed the mzk/VMenu-ShowMenu-refactoring branch from 4fb379e to 2bbb515 Compare December 30, 2023 00:08
@MKadaner
Copy link
Contributor Author

Thank you for the approval. I have been playing with it today for an hour or so and could not find any new issues. Let's call it a day.

I rebased and squashed. Please merge.

@alabuzhev alabuzhev merged commit 98c87ee into FarGroup:master Dec 30, 2023
25 of 27 checks passed
@alabuzhev
Copy link
Contributor

Ok. Thank you.

Copy link

sonarcloud bot commented Dec 30, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

14 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@MKadaner MKadaner deleted the mzk/VMenu-ShowMenu-refactoring branch December 30, 2023 00:22
@rohitab
Copy link
Contributor

rohitab commented Jan 3, 2024

@MKadaner sorry I wasn't able to respond sooner. I am going to upgrade to this version and will let you know if I come across any issues.

@MKadaner
Copy link
Contributor Author

MKadaner commented Jan 3, 2024

@rohitab, no problem at all. Your feedback is welcome at any time. When you upgrade, please pick the latest version. I pushed one more refactoring in the same area, drawing of the vertical lists to be more specific. The behavior should not be affected.

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.

3 participants