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

Diff view #330

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Diff view #330

wants to merge 3 commits into from

Conversation

lievenhey
Copy link
Contributor

No description provided.

@lievenhey lievenhey force-pushed the diff-view branch 2 times, most recently from ed32d25 to 786e577 Compare October 6, 2021 13:56
@lievenhey
Copy link
Contributor Author

image

@lievenhey
Copy link
Contributor Author

lievenhey commented Oct 6, 2021

The description of the table is not correct and the data might be a bit crappy

@milianw
Copy link
Member

milianw commented Oct 27, 2021

The description of the table is not correct and the data might be a bit crappy

is this still open or did you fix it now?

src/diffviewwidget.cpp Outdated Show resolved Hide resolved
src/diffviewwidget.h Outdated Show resolved Hide resolved
src/models/costdelegate.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,22 @@
void action1()
Copy link
Member

Choose a reason for hiding this comment

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

dito

src/models/diffviewproxy.cpp Outdated Show resolved Hide resolved
src/models/diffviewproxy.h Outdated Show resolved Hide resolved
src/models/diffviewproxy.h Outdated Show resolved Hide resolved
src/models/diffviewproxy.h Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the diff-view branch 4 times, most recently from 23916f1 to 04a1546 Compare November 5, 2021 12:39
@lievenhey
Copy link
Contributor Author

I know I still need to update the test files, but the approach taken to create the diff view should be better now.

@lievenhey lievenhey force-pushed the diff-view branch 3 times, most recently from 8bc5486 to 4f7a309 Compare November 8, 2021 11:14
@lievenhey
Copy link
Contributor Author

I tried adding diffing to the perfparser, but I don't know if this is the right way. Please tell me if the current way is acceptable or if I should try another one.

src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.h Outdated Show resolved Hide resolved
src/models/diffviewproxy.cpp Outdated Show resolved Hide resolved
src/models/treemodel.cpp Outdated Show resolved Hide resolved
src/mainwindow.h Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.h Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the diff-view branch 2 times, most recently from cd7a4fa to 3752c0a Compare November 16, 2021 14:17
@lievenhey
Copy link
Contributor Author

Still not done, I just wanted to save my progess

src/mainwindow.h Outdated Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

nearly done, this is looking really good now, great work and idea with just adding the diff as another cost - I really like that!

src/mainwindow.h Outdated Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Show resolved Hide resolved
src/parsers/perf/perfparser.h Outdated Show resolved Hide resolved
src/parsers/perf/perfparser.cpp Outdated Show resolved Hide resolved
@lievenhey lievenhey force-pushed the diff-view branch 2 times, most recently from 24193bc to e385487 Compare December 15, 2021 12:32
src/diffdialog.h Outdated Show resolved Hide resolved
src/diffdialog.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Show resolved Hide resolved
Copy link
Member

@milianw milianw left a comment

Choose a reason for hiding this comment

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

can you please squash the patches and add test coverage as well as a meaningful commit message. I would expect a basic summary therein (remember, the What and Why is important):

  • which part of the perf diff feature set is supported?
  • what problem does the diffcostproxy solve?
  • how does on use this new feature?

furthermore, please also add a quick way to diff files from the command line, something like

hotspot --diff perf.data.old perf.data

src/diffreportdialog.cpp Show resolved Hide resolved
src/diffreportdialog.cpp Outdated Show resolved Hide resolved
src/diffreportdialog.cpp Outdated Show resolved Hide resolved
src/diffreportdialog.cpp Outdated Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
src/models/data.cpp Outdated Show resolved Hide resolved
src/resultsbottomuppage.h Outdated Show resolved Hide resolved
src/models/costproxy.h Outdated Show resolved Hide resolved
src/models/costproxy.h Show resolved Hide resolved
src/resultstopdownpage.h Outdated Show resolved Hide resolved
@GitMensch
Copy link
Contributor

Other than the missing tests the docs would need an update.
If I knew how to test that locally and there's a CI build providing an AppImage of this I'd give this a test.

@lievenhey lievenhey marked this pull request as ready for review November 16, 2022 15:13
@lievenhey lievenhey force-pushed the diff-view branch 6 times, most recently from 7e57a77 to 8f94380 Compare November 18, 2022 16:06
@lievenhey lievenhey force-pushed the diff-view branch 3 times, most recently from f73e0ef to ce85f08 Compare December 8, 2022 12:30
@lievenhey lievenhey force-pushed the diff-view branch 4 times, most recently from 50599ac to c0c01a5 Compare December 8, 2022 15:32
@GitMensch
Copy link
Contributor

Can you please update the docs (README and possibly a screenshot), too?

@lievenhey lievenhey marked this pull request as draft September 19, 2023 14:48
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