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

Removing QWidget dependency from src/core. #6280

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

louib
Copy link
Member

@louib louib commented Mar 14, 2021

Fixes #1714

Testing strategy

Unit tests. Will also do some manual testing.

Type of change

  • ✅ Refactor (significant modification to existing code)

src/gui/DatabaseIcons.cpp Outdated Show resolved Hide resolved
tests/gui/TestGuiPixmaps.cpp Outdated Show resolved Hide resolved
@louib louib added this to the v2.7.0 milestone Mar 14, 2021
@louib louib force-pushed the remove_qwidget_from_core branch from 94a6d9c to 21ee278 Compare April 18, 2021 01:35
@droidmonkey
Copy link
Member

I need to get this merged...

@louib
Copy link
Member Author

louib commented Apr 18, 2021

@droidmonkey there's still a few things I need to figure out. I'm not sure where what to do with the QPixmapCache

@droidmonkey
Copy link
Member

I'll give it a looksie

@louib
Copy link
Member Author

louib commented Apr 18, 2021

@droidmonkey also it looks like the keepassxc-cli executable still depends on libQt5Widgets.so.5, libQt5Gui.so.5. I think this might require creating a dedicated CMake file for the core libraries. We could also consider moving all the core folders (like crypto/ and format/) into core/, to make it easier to identify what is core and what isn't. I guess this is could be part of the libkdbx initiative though, no need to do it in this PR.

@droidmonkey droidmonkey added the pr: refactoring Pull request that refactors code label Oct 11, 2021
@louib louib force-pushed the remove_qwidget_from_core branch 5 times, most recently from 7b9ed30 to 5830d52 Compare October 11, 2021 18:33
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #6280 (70c2d37) into develop (6f5bbf7) will decrease coverage by 0.02%.
The diff coverage is 60.68%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6280      +/-   ##
===========================================
- Coverage    64.16%   64.14%   -0.02%     
===========================================
  Files          333      333              
  Lines        42051    42018      -33     
===========================================
- Hits         26978    26949      -29     
+ Misses       15073    15069       -4     
Impacted Files Coverage Δ
src/autotype/AutoTypeMatchModel.cpp 0.00% <0.00%> (ø)
src/core/Global.h 100.00% <ø> (ø)
src/core/Tools.cpp 71.20% <ø> (+3.26%) ⬆️
src/core/Tools.h 0.00% <ø> (ø)
src/fdosecrets/widgets/SettingsModels.cpp 44.27% <0.00%> (ø)
src/gui/DatabaseIcons.cpp 54.24% <0.00%> (ø)
src/gui/DatabaseIcons.h 100.00% <ø> (ø)
src/gui/DatabaseTabWidget.cpp 64.05% <ø> (ø)
src/gui/HtmlExporter.cpp 0.00% <0.00%> (ø)
src/gui/IconDownloaderDialog.cpp 0.00% <0.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f5bbf7...70c2d37. Read the comment docs.

@louib
Copy link
Member Author

louib commented Oct 11, 2021

@droidmonkey @phoerious I've rebased this one, it's ready for another round of reviews. Let me know what you think about the QPixMap caching situation, and I'll update the two tests.

@louib louib marked this pull request as ready for review October 11, 2021 19:06
@louib louib force-pushed the remove_qwidget_from_core branch from 5830d52 to c7601be Compare November 6, 2021 16:04
@louib
Copy link
Member Author

louib commented Nov 6, 2021

@droidmonkey Is that the random testgui failure you mentioned in the Matrix channel?

FAIL!  : TestGui::testDatabaseSettings() Compared values are not the same

@droidmonkey
Copy link
Member

Yes these "test if the database saved correctly" checks need to be made more robust.

@louib louib mentioned this pull request Nov 10, 2021
5 tasks
@louib louib force-pushed the remove_qwidget_from_core branch from c7601be to d137f90 Compare November 10, 2021 02:46
@droidmonkey droidmonkey force-pushed the remove_qwidget_from_core branch from d137f90 to 30c403d Compare November 11, 2021 16:03
src/gui/Icons.cpp Outdated Show resolved Hide resolved
tests/gui/TestGuiPixmaps.cpp Outdated Show resolved Hide resolved
src/gui/DatabaseIcons.cpp Outdated Show resolved Hide resolved
@droidmonkey droidmonkey force-pushed the remove_qwidget_from_core branch from 30c403d to 70c2d37 Compare November 11, 2021 23:11
@droidmonkey
Copy link
Member

I made the fixes and removed some unnecessary functions in Icon.cpp. This is ready for merging IMO.

@droidmonkey droidmonkey merged commit 004f2b6 into develop Nov 12, 2021
@droidmonkey droidmonkey deleted the remove_qwidget_from_core branch November 12, 2021 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: refactoring Pull request that refactors code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow building the cli without linking Qt5::Widgets
4 participants