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

Compression UI utilities #683

Merged
merged 4 commits into from Feb 14, 2024

Conversation

hernanmd
Copy link
Collaborator

@hernanmd hernanmd commented Feb 8, 2024

Re-take of PR's causing dependency problems in the build.
In this PR, a new package is proposed: NewTools-Compression-Utils.

The new package includes a couple of minimal method to add open directory dialog to Compression classes.
It also removes the UIManager dependency from Compression.

These changes were the same as the previous PR's, but does instead of using the SpApplication hook, it uses directly the new FileBrowser API.

@hernanmd
Copy link
Collaborator Author

hernanmd commented Feb 8, 2024

Failing test testTransformingDeprecation is not related.

@hernanmd hernanmd added the enhancement New feature or request label Feb 8, 2024
Copy link
Contributor

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Looks like this approach will work better, but I'll wait for @estebanlm here :)
This adds a dependency from NewTools to Compression, which is understandable to me, and could be further refactored later. But maybe Esteban is thinking on a different package/project/repo organization.

@Ducasse
Copy link
Contributor

Ducasse commented Feb 8, 2024

This looks good to me because the UI and core of compression have been separated.

@Ducasse
Copy link
Contributor

Ducasse commented Feb 8, 2024

This is strange that we still get this traits fileout bug

@hernanmd
Copy link
Collaborator Author

hernanmd commented Feb 8, 2024

This is strange that we still get this traits fileout bug

Yes, I have to unselect definitions from Iceberg before commiting but some can slip away :(

@guillep
Copy link
Contributor

guillep commented Feb 8, 2024

Seems my PR solving that was not yet integrated :)

pharo-project/pharo#15978

There is an issue and I have to check it.
I'll try to do it quickly.

@Ducasse Ducasse requested review from estebanlm and removed request for MarcusDenker February 14, 2024 08:07
@estebanlm
Copy link
Member

I can't be sure the remaining error is because of the PR or inherited.
Please review :)

@guillep
Copy link
Contributor

guillep commented Feb 14, 2024

I can't be sure the remaining error is because of the PR or inherited.

I re-run the job to see if we can discard the second option.

Please review :)

Could you also give us your opinion on the issue and the package structure here?
Besides the broken test is the rest ok?

@estebanlm
Copy link
Member

yeah, is ok. I would not use a symbol to declare a group because it is not how the other groups are defined, but that is just a detail.

Copy link
Contributor

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Ok! Now let's wait for the CI.

src/BaselineOfNewTools/BaselineOfNewTools.class.st Outdated Show resolved Hide resolved
Copy link
Member

@estebanlm estebanlm left a comment

Choose a reason for hiding this comment

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

those errors do not seem related

Copy link
Contributor

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Hi @hernanmd

I think we are good to go.
Can you check my comment? The extensions that were moved here should be removed from Pharo (otherwise they will be duplicated and iceberg will confuse people :) )

@@ -0,0 +1,26 @@
Extension { #name : 'InflateStream' }

{ #category : '*NewTools-Compression-Utils' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this method was moved here, is there a "sister" PR in the Pharo repository removing the extension from there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, if there's no rush I will check it later today since now I'm about to start a meeting

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, it's not for yesterday :)

extent: 600 @ 800 ]
]

{ #category : '*NewTools-Compression-Utils' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@@ -0,0 +1,18 @@
Extension { #name : 'ZipArchive' }

{ #category : '*NewTools-Compression-Utils' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

@guillep
Copy link
Contributor

guillep commented Feb 14, 2024

Thanks @estebanlm ! I'll merge!

@guillep guillep merged commit ef9aad0 into pharo-spec:Pharo12 Feb 14, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants